-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Glimmer2] Add more curly component tests #13236
Conversation
This adds more tests are custom ids and bindings. These cases seem to be only tested by the `{{#view}}` helper and that is going away. To ensure we at least are tracking this functionality I added some tests to the curly-component integration tests.
@@ -18,6 +19,29 @@ moduleFor('Components test: curly components', class extends RenderingTest { | |||
this.assertComponentElement(this.firstChild, { content: 'hello' }); | |||
} | |||
|
|||
// Note this functionality seems weird |
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.
In {{foo-bar id=blah}}
, we evaluate the value of blah
and "freeze" it as elementId
in the component instance.
elementId
can't be changed, if it did change then any actions/events would stop working. The view registry is keyed on the elementId
, and is used to look up action/event targets. So it is definitely possible to make changing the id
/elementId
work, but it would be a new feature.
As a complete separate aside, we should ensure that a test also exists in this file for:
let fooBarInstance;
this.registerComponent('foo-bar', {
ComponentClass: Component.extend({
elementId: 'blahzorz',
init() {
this._super(...arguments);
fooBarInstance = this;
}
}),
template: `{{elementId}}`
});
this.render('{{foo-bar}}');
this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { id: 'blahzorz' }, content: 'blahzorz'});
this.runtask(() => {
expectAssertion(() => {
fooBarInstance.set('elementId', 'herpyderpy');
}, /can not change elementId/);
});
this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { id: 'blahzorz' }, content: 'blahzorz'});
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.
Makes sense. Added this test. The only thing is that an EmberError
gets thrown so expectAssertion
is not the correct assertion as it won't be stripped in prod builds.
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.
Ahh, gotcha. I wasn't sure if it was an assertion or an error.
This looks good, thanks for moving these tests. I left a comment explaining the behavior of |
8697de1
to
58f181a
Compare
35c7f0a
to
cf9d607
Compare
@rwjblue I think CI just needs to be kicked on this one. I had to do some interesting stuff to get the prod tests to pass. https://github.com/emberjs/ember.js/pull/13236/files#diff-c47395a5aaf92aa0a7edbfa812d4e0afR61 |
|
||
this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { id: 'blahzorz' }, content: 'blahzorz' }); | ||
|
||
if (EmberDev && !EmberDev.runningProdBuild) { |
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.
If only https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/views/states/in_dom.js#L24 used assert
this wouldn't be needed (you could use expectAssertion
which does these checks internally).
This is good though, I'm just grumbling 😈
Kicked sauce labs job... |
This adds more tests around custom ids and bindings. These cases seem to be
only tested by the
{{#view}}
helper and that is going away. To ensurewe are at least tracking this functionality I added some tests to the
curly-component integration tests.