-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Add Ember.run.callback() #115
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
- Start Date: 2016-1-28 | ||
- RFC PR: | ||
- Ember Issue: | ||
|
||
# Summary | ||
|
||
Add `Ember.run.callback()` to support running asynchronous code which tests (specifically `andThen()`) will wait for before considering the app "settled". | ||
|
||
# Motivation | ||
|
||
Ember's asynchronous test helpers (i.e. `visit()`, `fillIn()`, `andThen()`) provide a robust approach to acceptance testing in the face of asynchronous user interactions and application code. The `andThen()` helper is the backbone of this approach - it waits until the app has "settled" (i.e. async activity has competed) before asserting the desired state. | ||
|
||
For many, if not most, ambitious web applications, integration with third party libraries that require asynchronous interaction is a requirement. But unfortunately, there is currently no elegant solution for informing Ember of this asynchronous activity without resorting to lower level APIs like `registerWaiter()`. | ||
|
||
For such a common problem, Ember's opinionated approach needs an opinionated answer that matches the elegance of the rest of the test API. | ||
|
||
# Detailed design | ||
|
||
This RFC adds a single method to the `Ember.run` namespace: `Ember.run.callback()` (better naming suggestions are welcome!). | ||
|
||
The method takes a single function as an argument, and returns a wrapper function that can be used in place of the originally supplied one. This wrapper does two things: | ||
|
||
1. Ensures the callback is wrapped in an `Ember.run()` call, mostly for convenience. | ||
2. More importantly, it wraps the user-supplied function with a `registerWaiter` method (and the associated state management / cleanup) which resolves once the | ||
callback is invoked. | ||
|
||
By having the application register the waiters itself, we can avoid compromising the "purity" of the acceptance tests which would otherwise need to reach into the application's running state. | ||
|
||
The return value of `Ember.run.callback()` would be the wrapped function, but the waiter would be registered immediately. If the callback is no longer needed, the user could cancel the waiter by passing the callback to `Ember.run.cancel()` (matching the other async `Ember.run.*` methods). | ||
|
||
The implementation has been spiked out in an addon: [ember-run-callback](https://github.com/davewasmer/ember-run-callback). | ||
|
||
# Drawbacks | ||
|
||
* This would increase the `Ember.run` API surface and complexity, which (anecdotally) seems to be one of the less understood APIs in Ember. | ||
* There could be some potentially tricky edge cases if a user creates a test-aware callback via this method, but then fails to invoke it. Because invoking `Ember.run.callback()` _immediately_ starts to block the app from settling, if the callback is never invoked, it would hang the tests. | ||
|
||
# Alternatives | ||
|
||
This could always stay a userland addon rather than be adopted into the core framework. | ||
|
||
## Current solutions | ||
|
||
There are two main approaches to dealing with this problem currently: | ||
|
||
### Manual waiters | ||
|
||
```js | ||
click('.start-something-async'); | ||
Ember.Test.registerWaiter(function myCustomWaiter() { | ||
return find('.some-async-result').text().trim() === 'All done!'; | ||
}); | ||
andThen(function() { | ||
Ember.Test.unregisterWaiter(myCustomWaiter); | ||
expect(find('.some-async-result').text().trim()).to.equal('All done!'); | ||
}); | ||
``` | ||
|
||
### Direct referencing | ||
|
||
```js | ||
click('.start-something-async'); | ||
andThen(function() { | ||
let promise = app.__container__.lookupFactory('controller:foo').get('promise'); | ||
promise.then(function() { | ||
expect(find('.some-async-result').text().trim()).to.equal('All done!'); | ||
}); | ||
}); | ||
``` | ||
|
||
# Unresolved questions | ||
|
||
* Are there any performance implications of wrapping the callback with test related code? | ||
* Does this cover enough async use cases to warrant adoption, or are there significant use cases that would be missed? |
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.
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.
Run is part of metal, and register waiter is part of test, i am unclear if this should be part of run namespace.
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.
That's fair. My thought was that the behavior here matches similar behavior with the other Ember.run.* methods (i.e. want an Ember aware setTimeout?
run.later
. Want an Ember aware callback?run.callback
). It mixes concerns under the hood, but from my perspective when trying to solve this problem,Ember.run
was my gut reaction first place to check.Certainly open to suggestions of a better spot for it to live.