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

chore(dependencies): update bluebird v.3 #1904

Merged
merged 2 commits into from
Feb 18, 2016

Conversation

BridgeAR
Copy link
Contributor

Fixes #1859

I can't see that the cancellation is needed in this case as the promise that is cancelled is a runaway promise anyway.

@BridgeAR
Copy link
Contributor Author

Ping @dignifiedquire

@dignifiedquire
Copy link
Member

This is not enough, there is more cancellation happening in that file that will fail with the Bluebird@3. See for some details where I have started work on this. Also it is important that the cancellation continues to work on a refresh while doing a refresh for performance reasons

@BridgeAR
Copy link
Contributor Author

There is no other cancellation happening in the whole karma library. There might be peer dependencies that use cancellation but not in here anymore.

There is a CancellationError thrown but this is just the error itself that is thrown without any cancellation happening there. So this will work just as before.

I saw your work and my first attempt to solve this looked almost the same. But this is not the solution to this, as the way how it worked earlier is the same as how I implemented it right now. Earlier, the promise was cancelled and the other promise was returned. But that promise ended in nirvana, as it is not truly responsible for the refresh. That is handled in the .refresh instead of the ._refresh function and that returns the promise that the refresh is listening to.

So if I'm not really mistaken, the cancellation never had any effect and there's already a test in place that does a refresh while a refresh is ongoing and this would not pass otherwise.

@dignifiedquire
Copy link
Member

I see, but if do not cancel, won't this line https://github.com/BridgeAR/karma/blob/master/lib/file-list.js#L201 still be called? resulting in multiple emits even though the earlier one should have been canceled and not be emitted?

@BridgeAR
Copy link
Contributor Author

True point. Fixed and improved the test to catch that

@dignifiedquire
Copy link
Member

Thanks will review closely later today

@dignifiedquire
Copy link
Member

I think this doing now what we want. Thanks a lot for being patient with me :)

@dignifiedquire
Copy link
Member

Just one thing can you change the commit messages please, the first to sth feat(file-list): Upgrade bluebird to v3 and the second to sth like test(unit): ..

@BridgeAR
Copy link
Contributor Author

@dignifiedquire of course :) Fixed

@dignifiedquire
Copy link
Member

Thanks :octocat:

dignifiedquire added a commit that referenced this pull request Feb 18, 2016
chore(dependencies): update bluebird v.3
@dignifiedquire dignifiedquire merged commit e4b640a into karma-runner:master Feb 18, 2016
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