-
-
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
Implement RFC#995, Deprecate component template resolution #20660
Conversation
1ea6b41
to
95e4c1e
Compare
packages/@ember/-internals/glimmer/lib/component-managers/curly.ts
Outdated
Show resolved
Hide resolved
@@ -811,7 +830,7 @@ if (ENV._DEBUG_RENDER_TREE) { | |||
name: 'hello-world', | |||
args: { positional: [], named: { name: 'first' } }, | |||
instance: null, | |||
template: 'my-app/templates/components/hello-world.hbs', |
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.
this field is based of moduleName
, which we've talked about getting rid of.
It still works when a template is resolved, but when using setComponentTemplate
, there is no module name to associate -- could come from anywhere
@mixonic volunteered to help review this |
['@test lifecycle hooks during component append'](assert) { | ||
[`${testUnless( | ||
DEPRECATIONS.DEPRECATE_COMPONENT_TEMPLATE_RESOLVING.isRemoved | ||
)} lifecycle hooks during component append`](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.
This seems like test coverage that we would still want to keep after removing the deprecation?
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 can't figure this out -- test is too tied to specific VM behaviorss, it seems
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 did figure something out by not using any of the same test apis, and requiring slightly different assertions due to using the whole rendering system instead of manually creating factories, but I think the intent of the test is maintained in the new way
['@test appending, updating and destroying a single component'](assert) { | ||
[`${testUnless( | ||
DEPRECATIONS.DEPRECATE_COMPONENT_TEMPLATE_RESOLVING.isRemoved | ||
)} appending, updating and destroying a single component`](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.
Similarly, I think we want to keep this test after deprecation removal?
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.
for all of the append tests: #20660 (comment)
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.
the newly added test covers these use cases (destroy and update)
['@test appending, updating and destroying multiple components'](assert) { | ||
[`${testUnless( | ||
DEPRECATIONS.DEPRECATE_COMPONENT_TEMPLATE_RESOLVING.isRemoved | ||
)} appending, updating and destroying multiple components`](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.
ditto
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.
the newly added test covers these use cases
packages/@ember/-internals/glimmer/lib/component-managers/curly.ts
Outdated
Show resolved
Hide resolved
42c360d
to
265617e
Compare
…templates once the deprecation is in the removed state
6a59827
to
eb8fc81
Compare
RFC: emberjs/rfcs#995
Supersedes: ember-cli/ember-resolver#971