-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[CLEANUP] Remove DEPRECATE_COMPONENT_TEMPLATE_RESOLVING #20881
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
Conversation
| ); | ||
| } | ||
|
|
||
| [`${testUnless( |
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.
@NullVoxPopuli can you confirm that these tests were not erroneously marked with the deprecation? At a glance it seems like they were coincidentally using the deprecated behavior but I'm not sure.
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.
These are testing lifecycle hooks -- so perhaps they could be adapted to a non-deprecated template.
We'd probably want to keep these tests around until we get rid of @ember/component (or move it out to a library somewhere)
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.
Do you know how to adapt them to not use the deprecated feature? If you put an example here @tcjr may be able to resurrect those tests.
* Port the sizes workflow from glimmer-vm * Explicity set permission * Adjust permissions * Use pull_request_target for safety * pull_requset_target => pull_request * Try to not be vulnerable to basic attacks in GH Actions * Don't upload the whole dist as an artifact * Cache main on main * Build on #main * Rename * Updates * Concat the run id * Fix path * Fix paths * paths
Fix size PR workflow
Fix size comment workflow
* AHHHHH * Test empty commit * Test empty commit * Test empty commit * Trigger Main CI * Test empty commit * Allow workflow_dispatch * Test empty commit * fix * Test empty commit * Fix run-id * what * what * AHHHH * AHHHH * AHHHH * AHHHH * AHHHH * AHHHH * AHHHH * AHHHH * AHHHH * Fix PR number saving * AHHHH * AHHHH * AHHHH * AHHHH * AHHHH * AHHHH * ope * All * Shrink output * Fix paths
* Diff dev assets * Add diff to size action * Fix diff * Fix diff * Update size * Demo * hm * hm * hm * hm * hm * hm * hm * hm * Revert "Demo" This reverts commit c520b6f.
| layoutName: 'components/x-parent', | ||
| }), | ||
|
|
||
| resolveableTemplate: |
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 we change this to template, and then unwrap the expectDeprecation thing, we should be good to go!
(keeping the tests, but also resolving 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 restored the tests (unchanged) in this file, removing the testUnless deprecation wrapper.
I'm not sure how to fix the failing tests; I'm still working on that. Just changing resolveableTemplate to template was insufficient (different resolution errors).
* Removed tests targeting DEPRECATE_TEMPLATE_ACTION * Remove code deprecated via DEPRECATE_TEMPLATE_ACTION * Restored non-modifier bits of the action modifier source file * Update packages/@ember/-internals/glimmer/lib/resolver.ts * Update packages/@ember/-internals/glimmer/lib/resolver.ts * Update packages/@ember/-internals/glimmer/lib/resolver.ts * Update packages/@ember/-internals/glimmer/lib/resolver.ts * Removed node test with action modifier --------- Co-authored-by: Katie Gengler <katie@kmg.io>
Development AssetsDiff --- main/out.txt 2025-03-28 16:02:56.000000000 +0000
+++ pr/./pr-14199927699/out.txt 2025-04-01 15:39:40.000000000 +0000
@@ -1,6 +1,6 @@
2.2M └─┬ .
1015K ├─┬ @ember
- 206K │ ├─┬ -internals
+ 205K │ ├─┬ -internals
69K │ │ ├─┬ views
64K │ │ │ └─┬ lib
23K │ │ │ ├── mixins
@@ -14,7 +14,7 @@
26K │ │ ├─┬ meta
21K │ │ │ └── lib
11K │ │ ├── owner
- 9.6K │ │ ├── deprecations
+ 9.4K │ │ ├── deprecations
7.4K │ │ ├── metal
7.0K │ │ ├── string
5.1K │ │ ├── glimmerDetails
|
|
@NullVoxPopuli Why would this PR increase the size of shared-chunks by 19K? |
|
it might not be rebased -- like main could have some other deprecation removed that is still present in this branch? |
|
@NullVoxPopuli ah I see -- wonder if we should gate the size comment on the PR being up-to-date? |
|
Since Ci/Tests run with an ephemeral "what would be if the PR were merged" branch, is there a way we can get that ref and use that? |
hm, github.ref should already be that on pull_request... |
…jr/ember.js into clean-up-component-template-resolving
|
@tcjr I think something has gone wrong here -- it is showing files as new that already exist 🤔 |
|
Related to #20816.
The removal of this deprecation means that the resolver's internal function
layoutForwould always returnnull. It's only called in one place, so I removed the function and updatedlookupComponentPairto usenullin place of the return fromlayoutFor.