Assert settled() only takes an object as first argument
#303
Conversation
|
Also addresses #300. After thinking about this more, let’s just make this work without error for now. In the long run, I want to deprecate the options argument completely and making this an assertion now then removing it later feels odd to me... |
|
That makes sense. So does that mean close the PR? Or just |
| @@ -200,5 +201,8 @@ export function isSettled() { | |||
| export default function settled() { | |||
| let options = arguments[0]; | |||
|
|
|||
| assert(`The 'settled' (formerly 'wait') testing helper only accepts 'undefined' or an object. You passed ${options}`, | |||
| typeof options === 'undefined' || typeof options === 'object' && options !== null); | |||
There was a problem hiding this comment.
why is undefined a valid value? settled() and settled(undefined) are somewhat similar but I don't think we should encourage passing undefined here.
There was a problem hiding this comment.
Because that’s what the value appears to be for settled(). I don’t think actually passing undefined is useful in anyway.
There was a problem hiding this comment.
yep, that is why I don't think we should mention it that way in the assertion message. also we could check if arguments.length > 0, because IMHO settled(undefined) should also throw the assertion.
There was a problem hiding this comment.
Oh I see. Sure, that makes sense.
|
@snewcomer - Ya, if you are interested in updating that would be great. Basically we should avoid the |
|
Another option here that is growing on me, is to treat the change in Thoughts? |
|
Ah great points! The line below had to be refactored b/c as a callback w/ And I like the idea of changing the wait helper to sanitize the input. Would it just simply ignore the passed in |
snewcomer
force-pushed
the
assertion-settled
branch
from
0df3eda to
48dbc39
Compare
|
Pinging this again. The test is actually passing I believe.
|
|
@snewcomer correct! The linter is failing, but the tests themselves are good: https://travis-ci.org/emberjs/ember-test-helpers/jobs/333478168#L500 You should be able to test fixes with |
snewcomer
force-pushed
the
assertion-settled
branch
from
d80a0c4 to
f3af6b8
Compare
|
@rwjblue pinging ya on this one if you still think is relevant :) |
| .then(settled); | ||
| .then(() => { | ||
| return settled(); | ||
| }); |
There was a problem hiding this comment.
you can still use short form: .then(() => settled());
| let options = arguments[0]; | ||
|
|
||
| if (arguments.length > 0) { | ||
| export default function settled(options) { |
There was a problem hiding this comment.
this was documentation will see that there is a parameter and document it as such, but as mentioned in #314 this parameter is kinda private so we should keep let options = arguments[0]; instead.
snewcomer
force-pushed
the
assertion-settled
branch
from
2007b44 to
2c279f5
Compare
Turbo87
changed the title
settled() only takes an object as first argument
snewcomer
force-pushed
the
assertion-settled
branch
from
8474fd3 to
f758272
Compare
|
I took a stab at an alternative strategy in #317, what do y'all think? |
Fixes #283