-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Use nextTick
and nextTickPromise
throughout codebase.
#269
Conversation
https://github.com/emberjs/ember-test-helpers/pull/269/files?w=1 makes this a bit easier to review, since most of the changes are indentation related (removed a level of nesting). |
@@ -146,7 +143,7 @@ export default function settled(options) { | |||
timeout = MAX_TIMEOUT; | |||
} | |||
|
|||
global.setTimeout(function() { | |||
nextTick(function() { |
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.
since this invocation is called with a non-0 timeout value does it make sense to call something called nextTick
here?
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.
I can export it with a different name. I agree it isn’t perfect.
The basic gist that we need to do is ensure we use the eagerly cached setTimeout
(which is what nextTick
actually is).
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.
sure, I understand why it's there and why it's used, I'm just not sure it is the best name. I like the name for the situation where you don't pass any timeout at all, but once you pass a timeout the name is not fitting anymore since it won't actually be the next tick 😉
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.
Yep, totally agree. I’ll export it as futureTick
also 😜 and use that here...
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.
👍 - Updated now.
* Updates `setupContext` and `setupRenderingContext` to use `nextTickPromise` (simplifying the code nicely). * Add `futureTick` util. * Updates `settled` to use `nextTick` and `futureTick` where appropriate. * Add `settled()` calls after `render`, `clearRender`, `teardownContext`, and `teardownRenderingContext` to ensure that all run-loop business has been completed before returning.
c84d302
to
7548b65
Compare
Updates
setupContext
andsetupRenderingContext
to usenextTickPromise
(simplifying the code nicely).Also adds
settled()
calls afterrender
,clearRender
,teardownContext
, andteardownRenderingContext
to ensure that all run-loop business has been completed before returning.Fixes #268.