-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Conversation
Implements RFC#1099 - renderComponent()
Copies over the work from
- Initial implementation of renderComponent #20781
But this PR was created before some VM updates, so the changes from the upgrade PR here
https://github.com/emberjs/ember.js/pull/20842/files
need to be re-applied to the work done in Initial implementation of renderComponent #20781
See RFC for differences between the original PR and this one:
9ecd8a4 to
c017b13
Compare
Estimated Asset SizesDiff --- main/out.txt 2025-08-13 03:31:42.000000000 +0000
+++ pr/./pr-17218239291/out.txt 2025-08-25 19:09:16.000000000 +0000 @@ -1,36 +1,36 @@ +-------+-----------+-----------+ | | Min | Gzip | +-------+-----------+-----------+ -| Total | 418.48 KB | 231.88 KB | +| Total | 418.58 KB | 232.08 KB | +-------+-----------+-----------+ +----------------------+-----------+-----------+ | @ember/* | Min | Gzip | +----------------------+-----------+-----------+ -| Total | 239.23 KB | 147.06 KB | +| Total | 239.33 KB | 147.09 KB | +----------------------+-----------+-----------+ -| -internals | 35.44 KB | 25.49 KB | +| -internals | 35.44 KB | 25.48 KB | | application | 12.83 KB | 7.62 KB | | array | 12.66 KB | 7.32 KB | | canary-features | 304 B | 419 B | -| component | 1.07 KB | 1004 B | +| component | 1.07 KB | 989 B | | controller | 1.8 KB | 1.36 KB | | debug | 11.4 KB | 7.92 KB | | deprecated-features | 31 B | 77 B | | destroyable | 561 B | 383 B | | enumerable | 259 B | 387 B | -| helper | 823 B | 570 B | +| helper | 823 B | 588 B | | instrumentation | 2.43 KB | 1.78 KB | -| modifier | 669 B | 614 B | +| modifier | 669 B | 585 B | | object | 33.78 KB | 20.79 KB | | owner | 159 B | 178 B | -| renderer | 385 B | 327 B | -| routing | 58.05 KB | 33.43 KB | +| renderer | 385 B | 314 B | +| routing | 58.05 KB | 33.4 KB | | runloop | 2.2 KB | 1.33 KB | | service | 859 B | 741 B | -| template | 430 B | 390 B | +| template | 430 B | 413 B | | template-compilation | 429 B | 366 B | -| template-compiler | 57.81 KB | 30.3 KB | +| template-compiler | 57.91 KB | 30.4 KB | | template-factory | 94 B | 160 B | | test | 923 B | 627 B | | utils | 3.93 KB | 3.5 KB | @@ -40,7 +40,7 @@ +-----------------+-----------+----------+ | @glimmer/* | Min | Gzip | +-----------------+-----------+----------+ -| Total | 179.25 KB | 84.81 KB | +| Total | 179.25 KB | 84.99 KB | +-----------------+-----------+----------+ | destroyable | 2.7 KB | 1.35 KB | | encoder | 596 B | 653 B | @@ -52,7 +52,7 @@ | owner | 159 B | 202 B | | program | 7.1 KB | 3.63 KB | | reference | 5.51 KB | 3.18 KB | -| runtime | 95.26 KB | 42.51 KB | +| runtime | 95.26 KB | 42.69 KB | | tracking | 989 B | 961 B | | util | 3.03 KB | 2.29 KB | | validator | 15.64 KB | 6.86 KB | Details
|
f0d7327 to
54845d7
Compare
commit bundles a change to the plugins that fixes lexical scope bugs.
I intend to separate those changes out into their own PR before
attempting to merge this one. This PR also needs a flag for the new API.
Finally this commit adds some new testing infrastructure to generalize
the base render tests so it can be used with templates that are not
registered into a container. This is useful more generally, and could
be used in other places in the test suite in the future.
54845d7 to
be16cb2
Compare
| buildOwner, | ||
| clickElement, | ||
| defComponent, | ||
| defineComponent, |
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.
what's the difference between defComponent and defineComponent?
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.
defineComponent is traditional loosemode and defComponent is strict only
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.
why not defineStrictComponent?
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.
idk -- this code was copied from a prior implementation attempt
It's ultimately not important right now, but I'm happy to rename this util in a separate PR
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 should clarify, yesterday I wasn't in a mood to push updates unless there were more comments to make pushing (and all of CI) worthwhile.
Apologies -- defineStrictComponent is just obectively better, and I'll rename. thank you for the suggestion! <3
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.
actually, I was wrong:
/**
* The signature of this test helper is closer to the signature of the
* `template()` function in `@ember/template-compiler`. It can express the same
* functionality as {@linkcode defineComponent}, but most tests still use
* `defineComponent`. Migrating tests from {@linkcode defineComponent} is
* straightforward, and there's no *semantic* reason not to do so.
*/
it seems I should punt on changes here and instead make template work in tests (and get rid of this helper)
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.
Rather than template(), I think it would be fine to go straight to in the tests.
The test suite runs in Vite, if it doesn't already have template tag support enabled adding it should be trivial.
| }); | ||
| } | ||
|
|
||
| '@skip when args are a custom tracked class, the rendered component response appropriately'() { |
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'm curious why this doesn't just naturally work?
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.
yea, I consider this not working as a bug -- i suspect it is because of how ArgsProxy relies on Object.keys and @tracked properties are not enumerable.
Fixing this may require changes to how we handle args / ArgsProxy
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.
Oof, yeah, non-enumerability of tracked always sneaks up on me. You don't notice it until you notice it.
ef4
left a 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.
I commented above on refactoring goal for tests and also asked a question about a skipped test, but neither of those is a blocker for this PR.