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

Adds a basic failing test for settled #458

Merged
merged 4 commits into from
Nov 7, 2018

Conversation

wagenet
Copy link
Member

@wagenet wagenet commented 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.

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.
@wagenet wagenet added the bug label Nov 2, 2018
@scalvert
Copy link
Contributor

scalvert commented 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 rwjblue force-pushed the test-is-settled branch 2 times, most recently from 1db86b6 to c260ad0 Compare November 2, 2018 23:45
@wagenet
Copy link
Member Author

wagenet commented Nov 5, 2018

Hmm... I can't reproduce the error locally, and it seem unrelated to the change.

@rwjblue rwjblue requested a review from Turbo87 November 5, 2018 17:54
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.
@wagenet
Copy link
Member Author

wagenet commented Nov 5, 2018

Looks like the failure was related to old FF, using Latest ESR now.

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

😩😫🙀
Copy link
Member

@hjdivad hjdivad left a 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

addon-test-support/@ember/test-helpers/-utils.ts Outdated Show resolved Hide resolved
Turbo87 and others added 2 commits November 6, 2018 16:22
Co-Authored-By: wagenet <peter.wagenet@gmail.com>
@rwjblue rwjblue merged commit b36fb34 into emberjs:master Nov 7, 2018
@wagenet wagenet deleted the test-is-settled branch November 7, 2018 16:23
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants