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

Fix bug in Fetch earliest offsets #572

Merged
merged 21 commits into from
Jan 30, 2017

Conversation

bkim54
Copy link
Contributor

@bkim54 bkim54 commented Jan 20, 2017

when offset isnt ready need to call proper method in setTimeout.

Copy link
Collaborator

@hyperlink hyperlink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I know the code is copied from before I never liked this timeout since there's no guarantee it will be ready within 100ms.

Could we add a once listener to the ready event instead? It seems like it would be more reliable.

@bkim54
Copy link
Contributor Author

bkim54 commented Jan 23, 2017

@hyperlink requested changes made but your tests fail due to the changes
image

@hyperlink
Copy link
Collaborator

Right, those tests should fail because they rely on the ready var being changed. Can you update the test to emit ready from client instead of changing the value? Emitting should also implicitly set the ready to true.

setTimeout(function () {
this.fetch(payloads, cb);
}.bind(this), 100);
this.once('ready', () => this.fetch(payloads, cb).bind(this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a bind here since we're using fat arrow.

setTimeout(function () {
this.commit(groupId, payloads, cb);
}.bind(this), 100);
this.once('ready', () => this.commit(groupId, payloads, cb).bind(this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here.

setTimeout(function () {
this.fetchCommits(groupId, payloads, cb);
}.bind(this), 100);
this.once('ready', () => this.fetchCommits(groupId, payloads, cb).bind(this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here.

@bkim54
Copy link
Contributor Author

bkim54 commented Jan 30, 2017

@hyperlink, good to go?

@hyperlink hyperlink merged commit 74d3d2a into SOHU-Co:master Jan 30, 2017
@hyperlink
Copy link
Collaborator

Published as 1.3.3.

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