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

Expose settled helper function. #223

Merged
merged 3 commits into from
Oct 17, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Oct 16, 2017

  • Rename wait to settled
  • Ensure existing import wait from 'ember-test-helpers/wait' code works properly
  • Export settled as public API from the main index.js.
  • Move existing wait tests into legacy-0-6-x folder
  • Duplicate and refactor existing wait tests to the new API style (setupRenderingContext, async/await, etc)

The primary reasoning for the rename from wait to settled:

test('can do xyz', async function(assert) {
  await this.render(hbs`{{some-thing}}`);
  await click('.some-thing');
  await wait(); // <- WAT?!?!!?
});

This looks much better:

test('can do xyz', async function(assert) {
  await this.render(hbs`{{some-thing}}`);
  await click('.some-thing');
  await settled();
});

* Rename `wait` to `settled`
* Ensure existing `import wait from 'ember-test-helpers/wait'` code works properly
* Export `settled` as public API from the main `index.js`
* Move existing `wait` tests into `legacy-0-6-x` folder

Primary reasoning for the rename from `wait` to `settled`:

```js

test('can do xyz', async function(assert) {
  await this.render(hbs`{{some-thing}}`);
  await click('.some-thing');
  await wait(); // <- WAT?!?!!?
});
```

This looks much better:

```js
test('can do xyz', async function(assert) {
  await this.render(hbs`{{some-thing}}`);
  await click('.some-thing');
  await settled();
});
```
let waitForWaiters = options.hasOwnProperty('waitForWaiters') ? options.waitForWaiters : true;

return new EmberPromise(function(resolve) {
let watcher = self.setInterval(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this use the new ./global util instead of self?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, will update.

_setupPromiseListeners,
_teardownAJAXHooks,
_teardownPromiseListeners,
} from './settled';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine for now, I'm just wondering if we should deprecate importing this module at some point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, totally agreed. Basically, I want to introduce all the new functionality without deprecating anything at first. Then as we get more sure of the new system (e.g. after a little while of using the new API’s by default) we add in all the deprecations.

I want to be very careful about deprecation spew leading in to Ember 3.0...

});

test('it waits for Ember test waiters', async function(assert) {
await this.render(hbs`{{x-test-5}}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since render() without this is apparently meant to be the documented default should we use it here too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, but I didn’t want to do that here since I wasn’t sure how you felt about the other PR yet.

@rwjblue rwjblue merged commit d2457ca into emberjs:master Oct 17, 2017
@rwjblue rwjblue deleted the migrate-wait-to-settled branch October 17, 2017 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants