-
-
Notifications
You must be signed in to change notification settings - Fork 257
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
Adds a basic failing test for settled
#458
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wagenet
added a commit
to wagenet/ember-test-helpers
that referenced
this pull request
Nov 2, 2018
This avoids the issue detailed in emberjs#458.
wagenet
added a commit
to wagenet/ember-test-helpers
that referenced
this pull request
Nov 2, 2018
This avoids the issue detailed in emberjs#458.
wagenet
added a commit
to wagenet/ember-test-helpers
that referenced
this pull request
Nov 2, 2018
This avoids the issue detailed in emberjs#458.
rwjblue
reviewed
Nov 2, 2018
:( |
As of Ember 3.2, `isSettled()` is no longer `true`, even immediately after `await settled()`. This is because `waitUntil` uses an RSVP Promise which resolves with a runloop. So when `waitUntil`'s promise is about to resolve, things _are_ settled, but as soon as we actually resolve, a new runloop is created which causes `isSettled()` to be `false`.
rwjblue
force-pushed
the
test-is-settled
branch
2 times, most recently
from
November 2, 2018 23:45
1db86b6
to
c260ad0
Compare
Hmm... I can't reproduce the error locally, and it seem unrelated to the change. |
wagenet
added a commit
to wagenet/ember-test-helpers
that referenced
this pull request
Nov 5, 2018
This avoids the issue detailed in emberjs#458.
Looks like the failure was related to old FF, using Latest ESR now. |
Turbo87
reviewed
Nov 6, 2018
krisselden
approved these changes
Nov 6, 2018
Long ago in a galaxy far far away, Ember forced RSVP.Promise to "resolve" on the Ember.run loop. At the time, this was meant to help ease pain with folks receiving the dreaded "auto-run" assertion during their tests, and to help ensure that promise resolution was coelesced to avoid "thrashing" of the DOM. Unfortunately, the result of this configuration is that code like the following behaves differently if using native `Promise` vs `RSVP.Promise`: ```js console.log('first'); Ember.run(() => Promise.resolve().then(() => console.log('second'))); console.log('third'); ``` When `Promise` is the native promise that will log `'first', 'third', 'second'`, but when `Promise` is an `RSVP.Promise` that will log `'first', 'second', 'third'`. The fact that `RSVP.Promise`s can be **forced** to flush synchronously is very scary! Now, lets talk about why we are configuring `RSVP`'s `async` below... --- The following _should_ always be guaranteed: ```js await settled(); isSettled() === true ``` Unfortunately, without the custom `RSVP` `async` configuration we cannot ensure that `isSettled()` will be truthy. This is due to the fact that Ember has configurate `RSVP` to resolve all promises in the run loop. What that means practically is this: 1. all checks within `waitUntil` (used by `settled()` internally) are completed and we are "settled" 2. `waitUntil` resolves the promise that it returned (to signify that the world is "settled") 3. resolving the promise (since it is an `RSVP.Promise` and Ember has configured RSVP.Promise) creates a new Ember.run loop in order to resolve 4. the presence of that new run loop means that we are no longer "settled" 5. `isSettled()` returns false 😭😭😭😭😭😭😭😭😭 This custom `RSVP.configure('async`, ...)` below provides a way to prevent the promises that are returned from `settled` from causing this "loop" and instead "just use normal Promise semantics". 😩😫🙀
rwjblue
force-pushed
the
test-is-settled
branch
from
November 6, 2018 22:25
cd162e5
to
0da35f4
Compare
hjdivad
approved these changes
Nov 6, 2018
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.
👍 appreciate the detailed explanation
Turbo87
approved these changes
Nov 6, 2018
wagenet
added a commit
to wagenet/ember-test-helpers
that referenced
this pull request
Nov 7, 2018
This avoids the issue detailed in emberjs#458.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As of Ember 3.2,
isSettled()
is no longertrue
, even immediatelyafter
await settled()
. This is becausewaitUntil
uses an RSVPPromise which resolves with a runloop. So when
waitUntil
's promiseis about to resolve, things are settled, but as soon as we actually
resolve, a new runloop is created which causes
isSettled()
to befalse
.