Skip to content
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

Merged
merged 32 commits into from Apr 25, 2024

Conversation

NullVoxPopuli
Copy link
Sponsor Contributor

@NullVoxPopuli NullVoxPopuli changed the title Deprecate component template resolution Implement RFC#995, Deprecate component template resolution Mar 13, 2024
@NullVoxPopuli NullVoxPopuli force-pushed the deprecate-template-lookup branch 2 times, most recently from 1ea6b41 to 95e4c1e Compare April 9, 2024 00:02
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review April 9, 2024 19:48
@kategengler kategengler requested a review from ef4 April 10, 2024 21:30
@@ -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',
Copy link
Sponsor Contributor Author

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

@ef4
Copy link
Contributor

ef4 commented Apr 12, 2024

@mixonic volunteered to help review this

packages/@ember/-internals/glimmer/lib/resolver.ts Outdated Show resolved Hide resolved
['@test lifecycle hooks during component append'](assert) {
[`${testUnless(
DEPRECATIONS.DEPRECATE_COMPONENT_TEMPLATE_RESOLVING.isRemoved
)} lifecycle hooks during component append`](assert) {
Copy link
Contributor

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?

Copy link
Sponsor Contributor Author

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

Copy link
Sponsor Contributor Author

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) {
Copy link
Contributor

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?

Copy link
Sponsor Contributor Author

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)

Copy link
Sponsor Contributor Author

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Sponsor Contributor Author

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

@NullVoxPopuli NullVoxPopuli force-pushed the deprecate-template-lookup branch 2 times, most recently from 42c360d to 265617e Compare April 22, 2024 23:44
@ef4 ef4 merged commit 5c8df04 into emberjs:main Apr 25, 2024
26 checks passed
@NullVoxPopuli NullVoxPopuli deleted the deprecate-template-lookup branch April 25, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants