[DRAFT] Explore removing templateFor from curly component manager#21285
Closed
NullVoxPopuli-ai-agent wants to merge 2 commits intoemberjs:mainfrom
Closed
[DRAFT] Explore removing templateFor from curly component manager#21285NullVoxPopuli-ai-agent wants to merge 2 commits intoemberjs:mainfrom
NullVoxPopuli-ai-agent wants to merge 2 commits intoemberjs:mainfrom
Conversation
The curly component manager now uses getComponentTemplate() to resolve templates, aligning with how Glimmer components work. The layout and layoutName properties are removed from Component. Changes: - curly.ts: templateFor() uses getComponentTemplate(component.constructor) instead of layout/layoutName properties - component.ts: remove layout and layoutName property declarations - rendering.ts, router-non-application.ts: render() uses template() from @ember/template-compiler/runtime instead of compile + layoutName - abstract-application.ts: remove compile method - Tests using layoutName rewritten to use setComponentTemplate - Tests specifically testing layout/layoutName behavior removed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
NullVoxPopuli-ai-agent
force-pushed
the
nvp/remove-templateFor-dynamic-layout
branch
2 times, most recently
from
fbbde80 to
5601996
Compare
| manager: RootComponentManager; | ||
| capabilities = capabilityFlagsFrom(ROOT_CAPABILITIES); | ||
| compilable = null; | ||
| compilable: CompilableProgram; |
Contributor
There was a problem hiding this comment.
does this need to be here? a some point we need to stop unwrapping templates
Contributor
Author
There was a problem hiding this comment.
Agreed — the unwrapTemplate chain is tech debt. For now it's needed because the VM's compilable field requires CompilableProgram, and getComponentTemplate returns a TemplateFactory. Removing the unwrap would mean changing the VM to accept template factories directly.
| tagName: '', | ||
| layoutName: '-top-level', | ||
| let TopLevel = template(templateStr, { | ||
| component: Component.extend(Object.assign({}, context, { tagName: '' })), |
Contributor
There was a problem hiding this comment.
we c an probably just use templateOnly() here / omit component entirely. This value isn't doing anything for us
Contributor
Author
There was a problem hiding this comment.
Tried using templateOnly() but the container's owner.lookup calls .create() on the registered factory, which templateOnly() doesn't have. Kept Component.extend({ tagName: '' }) but removed the unused context parameter since this render method is never called with one.
Contributor
There was a problem hiding this comment.
so we update that, too
NullVoxPopuli-ai-agent
force-pushed
the
nvp/remove-templateFor-dynamic-layout
branch
from
5601996 to
b98dfff
Compare
…agers
Fully removes the dynamicLayout capability and getDynamicLayout from
curly and root component managers.
curly.ts:
- Removed getDynamicLayout, templateFor, WithDynamicLayout interface
- Set dynamicLayout: false in CURLY_CAPABILITIES
- Removed unused imports (getOwner, getComponentTemplate, unwrapTemplate,
CompilableProgram, RuntimeResolver)
root.ts:
- RootComponentDefinition eagerly resolves template via getComponentTemplate()
- Uses a default empty template (precompileTemplate('')) when no template
is set, so compilable is never null
- Set dynamicLayout: false in ROOT_CAPABILITIES
- Removed getDynamicLayout from RootComponentManager
Template resolution for curly components now has one path: the resolver
provides it via getComponentTemplate() at lookup time. Root components
get their template eagerly in the definition constructor.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
NullVoxPopuli-ai-agent
force-pushed
the
nvp/remove-templateFor-dynamic-layout
branch
from
b98dfff to
252e9f4
Compare
Contributor
|
once we deprecate |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Investigation
Explored removing
getDynamicLayout/templateForfrom the curly component manager.Finding
getDynamicLayoutcannot be fully removed because it's needed for the root component rendered viarunAppend(), which bypasses the resolver entirely.The template resolution flow for curly components has two paths:
Named components (e.g.
<FooBar />in a template): The resolver'slookupComponentPaircallsgetComponentTemplate(component.class)and returns the template as part of the component definition. The VM uses this directly —getDynamicLayoutis never called.Root component (via
runAppendinthis.render()): The component is created directly and passed torunAppend, bypassing the resolver. The VM has no template from the resolver, so it falls back togetDynamicLayout→getComponentTemplate(component.constructor).What changed
templateFor()method, inlined logic intogetDynamicLayoutgetDynamicLayoutis still neededWhat would be needed to fully remove it
runAppendwould need to go through the resolver, ORgetComponentTemplate()directly when no template is provided, ORthis.render()in tests would need to stop usingrunAppendwith classic componentsBuilds on #21284.
🤖 Generated with Claude Code