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

Use nextTick and nextTickPromise throughout codebase. #269

Merged
merged 1 commit into from
Dec 15, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Dec 15, 2017

Updates setupContext and setupRenderingContext to use nextTickPromise (simplifying the code nicely).

Also adds settled() calls after render, clearRender, teardownContext, and teardownRenderingContext to ensure that all run-loop business has been completed before returning.

Fixes #268.

@rwjblue rwjblue requested a review from Turbo87 December 15, 2017 01:06
@rwjblue
Copy link
Member Author

rwjblue commented Dec 15, 2017

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() {
Copy link
Member

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?

Copy link
Member Author

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).

Copy link
Member

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 😉

Copy link
Member Author

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...

Copy link
Member Author

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.
@rwjblue rwjblue merged commit 2c6332d into emberjs:master Dec 15, 2017
@rwjblue rwjblue deleted the use-next-tick-properly branch December 15, 2017 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants