Refactor settled to avoid triggering a new run-loop. #263
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
The code that was added to ensure that all `promise.then` or
`promise.catch` callbacks are "flushed" before decrementing the counter
(which fixed a number of bugs where `settled` would accidentally become
`true` before the promise callbacks themselves could have a chance to
run), caused an issue when using the legacy `moduleForComponent` system
to render a template that itself kicked off an async request.
Prior to the changes made in this commit, when rendering would kick off
async (e.g. rendering a `{{parent.child}}` style path referencing an
ember-data async relationship) there would _already_ be an existing
`Ember.run.backburner.currentInstance` (which was created by the
`RSVP.Promise.resolve().then(...)` being removed in this commit) which
ultimately forced us down the path that meant `adapter.asyncStart()` /
`adapter.asyncEnd()` would not be called ([see
here](https://github.com/emberjs/ember.js/blob/v2.17.0/packages/ember-testing/lib/ext/rsvp.js#L11)).
This change works around that (admittedly niche) scenario by always
assuming that a `Promise.resolve()` will execute before the next
`setTimeout(cb, 0)` (which is true in both Ember.RSVP land and native
Promise land).
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.
The code that was added to ensure that all
promise.thenorpromise.catchcallbacks are "flushed" before decrementing the counter (which fixed a number of bugs wheresettledwould accidentally becometruebefore the promise callbacks themselves could have a chance to run), caused an issue when using the legacymoduleForComponentsystem to render a template that itself kicked off an async request.Prior to the changes made in this commit, when rendering would kick off async (e.g. rendering a
{{parent.child}}style path referencing an ember-data async relationship) there would already be an existingEmber.run.backburner.currentInstance(which was created by theRSVP.Promise.resolve().then(...)being removed in this commit) which ultimately forced us down the path that meantadapter.asyncStart()/adapter.asyncEnd()would not be called (see here).This change works around that (admittedly niche) scenario by always assuming that a
Promise.resolve()will execute before the nextsetTimeout(cb, 0)(which is true in both Ember.RSVP land and native Promise land).Fixes ember-cli/ember-cli-mocha#233
Fixes #259