-
-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export default function assertFunction(modifierName, maybeFunction) { | ||
if (typeof maybeFunction === 'function') return; | ||
|
||
throw new TypeError(`${modifierName} expected a function, instead received "${maybeFunction}"`); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import { module, test } from 'qunit'; | ||
import { setupRenderingTest } from 'ember-qunit'; | ||
import { render } from '@ember/test-helpers'; | ||
import { render, setupOnerror } from '@ember/test-helpers'; | ||
import hbs from 'htmlbars-inline-precompile'; | ||
|
||
module('Integration | Modifier | will-destroy', function (hooks) { | ||
|
@@ -43,4 +43,57 @@ module('Integration | Modifier | will-destroy', function (hooks) { | |
// trigger destroy | ||
this.set('show', false); | ||
}); | ||
|
||
test('provides a useful error on install', async function (assert) { | ||
assert.expect(1); | ||
|
||
// Setup error capturing | ||
setupOnerror(function (err) { | ||
assert.equal( | ||
err.toString(), | ||
`TypeError: did-destroy expected a function, instead received "undefined"` | ||
); | ||
}); | ||
|
||
await render(hbs` | ||
<div {{will-destroy this.nonExistentMethod}}></div> | ||
`); | ||
|
||
// Prevent double error on test teardown | ||
this.set('nonExistentMethod', () => {}); | ||
|
||
// Reset error capturing | ||
setupOnerror(); | ||
}); | ||
|
||
test('provides a useful error on destroy', async function (assert) { | ||
assert.expect(1); | ||
|
||
// Start with a valid function so that install works | ||
this.set('nonExistentMethod', () => {}); | ||
|
||
// Setup error capturing | ||
setupOnerror(function (err) { | ||
assert.equal( | ||
err.toString(), | ||
`TypeError: did-destroy expected a function, instead received "undefined"` | ||
); | ||
}); | ||
|
||
this.set('show', true); | ||
await render(hbs` | ||
{{#if this.show}} | ||
<div {{will-destroy this.nonExistentMethod}}></div> | ||
{{/if}} | ||
`); | ||
|
||
// Remove the function to trigger an error on destroy | ||
this.setProperties({ | ||
nonExistentMethod: undefined, | ||
show: false, | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gnclmorais looks like this test fails. Di you try you add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Do you think we could potentially run these tests on all versions except these? |
||
// Reset error capturing | ||
setupOnerror(); | ||
}); | ||
}); |
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