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 instance null when involve async destruction #6095

Merged
merged 1 commit into from Mar 9, 2016

Conversation

Projects
None yet
4 participants
@yiminghe
Copy link
Contributor

commented Feb 23, 2016

see tests in Files changed

componentWillUnmount: function() {
var self = this;
// some async animation
setImmediate(function() {

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 24, 2016

Contributor

setImmediate shouldn't be used, since it means we will never be able to run these tests in a real browser. https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate

Consider setTimeout instead.

This comment has been minimized.

Copy link
@yiminghe

yiminghe Feb 25, 2016

Author Contributor

setTimeout is mocked by jest, don't know how to simulate asynchronous in jest

This comment has been minimized.

This comment has been minimized.

Copy link
@cpojer

cpojer Feb 25, 2016

Contributor

I don't see a problem with this? You guys should upgrade to jasmine2 so you can use the done feature of jasmine2 for async testing. Otherwise waitsFor is fine, just ugly API. You can also do jest.runAllTimers() to advance them manually – that way you can get rid of the waitsFor.

@zpao

This comment has been minimized.

Copy link
@yiminghe

yiminghe Feb 25, 2016

Author Contributor

runAllTimers run function provided to setTimeout synchronously. This test case needs asynchronization

This comment has been minimized.

Copy link
@cpojer

cpojer Feb 25, 2016

Contributor

what kind of async? You can also do jest.runAllImmediates.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2016

This looks fine to me ( 👍 ) , just one minor change requested.

@yiminghe yiminghe force-pushed the yiminghe:fix-async-ref-destruct branch from ae5e2d8 to df095c0 Feb 25, 2016

@yiminghe

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2016

After checking jest, use jest.runAllTimers(); is fine. thanks @cpojer

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 25, 2016

@yiminghe updated the pull request.

@yiminghe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

ping @jimfb

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2016

@yiminghe Thanks for the ping! And thanks for the PR, this looks great! Might even be able to squeeze it into v15!

jimfb added a commit that referenced this pull request Mar 9, 2016

Merge pull request #6095 from yiminghe/fix-async-ref-destruct
fix instance null when involve async destruction

@jimfb jimfb merged commit 2f4a8e9 into facebook:master Mar 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.