-
Notifications
You must be signed in to change notification settings - Fork 444
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
Make whenNothingPending always async #222
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me aside from some duplicated logic!
Also to clarify the motivation for this PR, I'm guessing it's the following statement by @ericyhwang from #204 (comment)
Code should always be async (e.g. process.nextTick) or always be sync, not both.
and
Remove the sync workaround, make a separate PR for making whenNothingPending async consistently.
@ericyhwang , could this PR be merged now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for making that change removing the duplicated logic.
There seems to be a meaningful test case removed (see inline comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to merge! /cc @ericyhwang
@gkubisa This change looks good, but unfortunately now has merge conflicts. @ericyhwang I think this would be a nice improvement. In fact I just ran into issues around this async behavior today. |
This PR has been already open for over 9 months, so I have very little motivation to keep it up to date with the master branch. It's already merged into the Teamwork fork of ShareDB. |
Understood. It's unfortunate that this good work has not gotten the attention it deserves. |
Ah, I lost track of this, sorry - my email notification routing for the Share org wasn't properly set up at that point. I'll do some integration testing on my end, and if that all checks out, we can get it merged tomorrow. |
Potentially this nextTick can be removed sharedb/lib/client/connection.js Line 583 in 5394fa4
Although, it's unrelated because it's on the connection. This could be a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from Nate during PR review meeting, apologies for dropping the ball!
This is published as sharedb@1.0.0-beta.22 |
See #204 (comment)