Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Browserify compatibility and updates gun to 0.9.7 #44

Merged
merged 2 commits into from
Jan 14, 2018

Conversation

zrrrzzt
Copy link
Contributor

@zrrrzzt zrrrzzt commented Jan 13, 2018

Adds a browser property to package.jsonand 2 browser-specific files to make the module work with browserify.

@amark
Copy link
Collaborator

amark commented Jan 13, 2018

@PsychoLlama people in the community were trying to get indexedDB gun-level working with browserify and this was the first working solution thanks to @zrrrzzt 's excellent calculations.

@amark
Copy link
Collaborator

amark commented Jan 13, 2018

Shoot, there is 1 failing test in only Node v6 (not v7 or v8), from my quick peek I don't think this is the browserify/indexedDB fix, I wonder if it relates to my v0.9.7 code (there is a bug I'm currently fixing that my tests didn't catch that I'm working on now so I wonder if this is related.

But at the same time, it works fine in v7 and v8 which maybe isn't a me issue?

@zrrrzzt
Copy link
Contributor Author

zrrrzzt commented Jan 13, 2018

Can you re-run the test?

Tested v6.12.3 on my machine (with nvm) and everything passed

@amark
Copy link
Collaborator

amark commented Jan 13, 2018

@zrrrzzt that is the same version as the travis test so if it passed on your machine it should be fine here too. I reran it earlier, will rerun it again. Hmm. You windows?

@amark
Copy link
Collaborator

amark commented Jan 13, 2018

https://travis-ci.org/PsychoLlama/gun-level/jobs/328520960 it doesn't look like your fault, it just looks like gun isn't merging the 2 results, which is why I wonder if it is the new gun version and the current bug I'm in the middle of fixing.

@zrrrzzt
Copy link
Contributor Author

zrrrzzt commented Jan 13, 2018

Nope. I suspect your tests tries to compare a timestamp or something else that varies.
Ran the test 5 times now. 1 green, then the next 3 red and the last one green again.

Ran a couple of tests in a row on v8.9.4 as well. Same thing. Almost random if the test fails or passes.

@zrrrzzt
Copy link
Contributor Author

zrrrzzt commented Jan 14, 2018

Done more testing and it doesn't matter if you use gun 0.8 or 0.9.7. If run run the tests enough times they will fail.

This seems to be the problem: https://github.com/PsychoLlama/gun-level/blob/master/src/spec.js#L71

If you console log out res1, res2 and value there are no errors in res1/res2 in the failing tests. However the value is not merged. If I understand correct GUN is eventual consistent and I think the failing tests are results of testing the value before the consistency.

@amark
Copy link
Collaborator

amark commented Jan 14, 2018

@zrrrzzt you are exactly correct (man, I an consistently impressed by your thoroughness of testing/debugging/assessing things!), BUT even though gun is eventually consistent there is (and should be with gun-level too) and API-level contract with the developer that IF the data has been put and especially if they've received an acknowledgement on that, that when they do a read it will already be there.

You testing against 0.8 and 0.9.7 calms my concern a tad. IF it is Level then it could be that Level isn't replying "fast enough" to gun's default, we can experiment through changing:

https://github.com/PsychoLlama/gun-level/blob/master/src/spec.js#L87 (a few lines down from the one you linked to)

        makeGun().get(key).val(value => {
          expect(value).toContain({ success: true, data: true });
          done();
        }, {wait: 500}); // THIS IS THE CHANGED LINE

This will tell val to "wait" 500ms (a very long time) before triggering the callback, this will give ample/sufficient time for Level to reply with all records that need to be read/merged.

If we are still getting spotty/inconsistent results, then that might suggest there is a bug in gun directly.

Sorry that this issue is coming up on your commit when I know your commit isn't causing it (I'm re-running the Travis tests every once in a while).

I'm still comfortable PULLING your changes and treating this as a separate issue. Can I do that?

@amark
Copy link
Collaborator

amark commented Jan 14, 2018

Yupe, look, that 2nd rerun I did today just triggered it to succeed like you said.

This is still an issue (thanks for opening one with good code to test against), so I'm gonna go ahead and merge, @PsychoLlama , so we have good indexxed-DB level support, and then we need to address the #45 issue later.

@zrrrzzt you are wonderful, thank you so much for contributing so much help across a wide variety of modules so far! You're great, really a shining example of Open Source. :D

@amark amark merged commit b4a2371 into gundb:master Jan 14, 2018
@zrrrzzt
Copy link
Contributor Author

zrrrzzt commented Jan 15, 2018

Forgot to mention I also tested the wait approach. I'll try to write similar tests for vanilla GUN (if they don't exist) and that would help you locate the bugs

@amark
Copy link
Collaborator

amark commented Jan 15, 2018

@zrrrzzt phenomenal, thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants