-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Improve not-a-function error message #50
base: master
Are you sure you want to change the base?
Conversation
When the passed `fn` to the helper is not a function, this commit improves the error message users will see, hopefully helping with the debugging process. Fixes emberjs#23
@@ -84,4 +84,34 @@ module('Integration | Modifier | did-insert', function (hooks) { | |||
|
|||
assert.dom('.alert').hasClass('fade-in'); | |||
}); | |||
|
|||
test('provides a useful error on insert', async function (assert) { |
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.
@gnclmorais nitpick, maybe name it provides a useful error when provided not a function
?
Same for other tests.
Just so that mentally in future we may add some other "useful error on insert" for another use case
nonExistentMethod: undefined, | ||
show: false, | ||
}); | ||
|
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.
@gnclmorais looks like this test fails. Di you try you add await settled()
before setupOnerror()
so that it re-renders and emit error and only after that resets?
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.
I’ve tried a couple of things to make sure this would work (your suggestion was one of them), but no luck so far… Curiously, it only fails with three older versions of Ember:
Scenario ember-lts-3.8: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency │ Expected │ Used │ Type │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source │ ~3.8.0 │ 3.8.3 │ yarn │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘
Scenario ember-lts-3.12: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency │ Expected │ Used │ Type │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source │ ~3.12.0 │ 3.12.4 │ yarn │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘
Scenario ember-lts-3.16: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency │ Expected │ Used │ Type │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source │ ~3.16.0 │ 3.16.10 │ yarn │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘
Do you think we could potentially run these tests on all versions except these?
Fixes #23. When the passed
fn
to the helper is not a function, this commit improves the error message users will see, hopefully helping with the debugging process.First time contributing to an Ember project, so feel free to give me any feedback you have. Cheers!