Resolve Element for classic Ember addon .hbs components (<EsCard>-style)#19
Open
Resolve Element for classic Ember addon .hbs components (<EsCard>-style)#19
Element for classic Ember addon .hbs components (<EsCard>-style)#19Conversation
Classic Ember addon components ship a `.hbs` template (`addon/templates/components/<name>.hbs` or `app/components/<name>.hbs`) and have no JS-side `Signature['Element']`, no `satisfies TOC<…>`. JS-driven resolution returns null/'transparent' for them, so the consumer's children floated up past the runtime root and content-model rules FP-fired. `resolveAddonHbsTemplate` walks the import for the component, extracts the `moduleSpecifier` (`<addon>/components/<name>`), walks up from the consumer file to find `node_modules/<addon>`, probes the canonical Ember addon template paths, and parses the root native element via the existing `extractSplattedRootFromTemplate` helper. Both the rendered tag (for componentTagMap) and the splatted-root attrs (for componentAttrMap) flow through the same plumbing modern shapes use. Wired in as a *fallback* — only runs when JS-side resolution returned null or 'transparent' so modern shapes (class form, TOC forms, curried block-params) take priority. Surfaced by ecosystem CI on ember-website (98 findings, all <EsCard> from ember-styleguide) and HDS (~25 internal classic addon list-item components). Test plan: cross-file fixture test/glint-fixtures/node_modules/fake-card-addon/ shipping a real `.hbs` template at the canonical path; consumer imports from the package and uses inside <ul>; assertion verifies `componentTagMap` resolves to `'li'`.
There was a problem hiding this comment.
Pull request overview
Adds a Glint-resolution fallback to determine the rendered native element (and splatted-root attrs) for classic Ember addon components implemented only as .hbs templates, so content-model validation doesn’t incorrectly treat these components as “transparent” and float children into the parent.
Changes:
- Introduces
resolveAddonHbsTemplatefallback inextractAttrTypeMapwhen JS/type-driven resolution returnsnullor'transparent'. - Probes canonical Ember addon template locations in
node_modules/<addon>/...and parses the.hbsroot element viaextractSplattedRootFromTemplate. - Adds a cross-file fixture + test covering addon
.hbsroot tag resolution (<FakeCard>→li).
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/glint.ts |
Adds addon .hbs template fallback resolution (tag + splatted-root attrs) when Glint/TS element resolution is missing/transparent. |
test/glint.test.ts |
Adds a test asserting addon .hbs components resolve to their native root tag. |
test/glint-fixtures/fake-card-consumer.gts |
New fixture consuming an addon component via package import to exercise the fallback. |
test/glint-fixtures/node_modules/fake-card-addon/package.json |
Adds a fake addon package under fixture node_modules/ for test resolution. |
test/glint-fixtures/node_modules/fake-card-addon/addon/templates/components/fake-card.hbs |
Provides the .hbs template whose root element is parsed (<li ...attributes>). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review on #19: 1. Import-path regex was [\\w-]+, which excludes legitimate npm package names containing '.' (e.g. lodash.debounce). Widened to npm package-name rules: lowercase letters, digits, '.', '-', '_'; cannot start with '.' / '_'. Added an explicit '..' check to prevent path traversal even though the regex already excludes leading dots. 2. Memoize the (consumerDir, importPath) → ComponentAttrs|null lookup so repeated invocations of the same addon component skip the directory walk + multiple existsSync probes + read+parse. Cache covers both positive and negative resolutions; consumerDir is included in the key so a monorepo with multiple node_modules trees stays correct. _clearAddonHbsCache exposed for tests.
Per Copilot review on #19: 1. fs.readFileSync(hbsPath, 'utf8') wrapped in try/catch. Even with the existsSync guard above it, the read can still fail (TOCTOU race after exists-check, permission errors, unreadable file). Other reads in the module already handle failures gracefully; this one now matches. 2. Tightened the component-name regex. Comment said 'kebab-case' but the previous regex `[\\w/-]+` allowed uppercase letters and underscores (\\w = [A-Za-z0-9_]). Now strictly lowercase + digits + hyphens + slashes (for nested names like `forms/text-input`), matching the comment and Ember's actual addon convention.
Per Copilot review on #19: the previous loop guard `while (dir !== path.dirname(dir))` exited before checking the root — e.g. `/node_modules/<addon>` on POSIX or `C:\\node_modules\\<addon>` on Windows would never be probed, diverging from Node's module resolver. Restructured as a `for (;;)` that checks the current dir, then breaks if dir is its own parent (the root).
Per Copilot review on #19: 1. Cache invalidation. `addonHbsResolutionCache` previously stored `null` for every negative path: regex non-match, missing `node_modules/<addon>`, no `.hbs` at any of the three probed locations, and read failure post-existsSync. In long-lived processes (IDE, watch mode) or with linked workspace addons, that permanently hid templates that later appeared. Switched to positive-only caching: re-probe negatives every call (cheap — regex + handful of existsSync up to root); cache the file read + Glimmer parse on hit (the actually-expensive step). Cache type tightened to `Map<string, ComponentAttrs>` so positive-only is enforced by the type system. 2. Expanded fixture coverage. Existing test only covers `<addon>/components/<name>` import + `addon/templates/components/` probe path with an unscoped package. Added a second fixture exercising three new dimensions in one shot: - `@scope/foo-addon` (scoped package — different regex branch) - `@scope/foo-addon/templates/components/scoped-card` (the OTHER accepted import shape, `<addon>/templates/components/<name>`) - `app/components/scoped-card.hbs` (the second of the three probed paths inside the addon root) - Root element `<section>` to keep tag distinct from `<li>`. 129/129 pass. No behavioral change beyond the cache lifetime.
Per Copilot review on #19: 1. The cache docstring was internally inconsistent — it described the map as "ComponentAttrs (or null for negative resolutions)" while the implementation had been switched to positive-only `Map<string, ComponentAttrs>` in the previous round. Consolidated the two stale docstring blocks into a single accurate one; clarified positive-only semantics and noted the interaction with the disk cache in `lib/cache.ts` (which keys by consumer file content + mtime, so a hot-installed addon may not be picked up until the consumer file changes). 2. `extractSplattedRootFromTemplate` can return any ElementNode tag it encounters, including PascalCase component names. An addon whose `.hbs` root is itself a component invocation (`<AnotherComponent ...attributes>{{yield}}</AnotherComponent>`) would otherwise feed `AnotherComponent` into `componentTagMap`, and blank.ts's substitution path would rename the consumer's invocation to that non-native tag — emitting a non-HTML tag into the validated output and making content-model checks worse than the transparent-blanking fallback. Gate the cache+return on `isNativeTag(result.tag)`; non-native tags fall through to transparent. New regression fixture `composed-card-consumer.gts` + addon `composing-addon` mirrors the composing-addon shape and asserts `AnotherComponent` does NOT land in `componentTagMap`. 130/130 pass.
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
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.
Summary
Classic Ember addon components ship a
.hbstemplate (addon/templates/components/<name>.hbsorapp/components/<name>.hbs) and have no JS-sideSignature['Element'], nosatisfies TOC<…>. JS-driven resolution returnsnull/'transparent'for them, so the consumer's children floated up past the runtime root and content-model rules FP-fired.resolveAddonHbsTemplatewalks the import for the component, extracts themoduleSpecifier(<addon>/components/<name>), walks up from the consumer file to findnode_modules/<addon>, probes the canonical Ember addon template paths, and parses the root native element via the existingextractSplattedRootFromTemplatehelper. Both the rendered tag (forcomponentTagMap) and the splatted-root attrs (forcomponentAttrMap) flow through the same plumbing modern shapes use.Wired in as a fallback — only runs when JS-side resolution returned
nullor'transparent', so modern shapes (class form, TOC forms, curried block-params) take priority.Probed paths
In order:
<addon>/addon/templates/components/<name>.hbs(legacy v1 addon)<addon>/app/components/<name>.hbs(Module Unification / authored-as-app shim)<addon>/addon/components/<name>.hbsImport-path matching accepts:
<addon>/components/<name><addon>/templates/components/<name>@org/pkg/components/<name>)<addon>/components/forms/text-input)Surfaced by
Ecosystem CI:
ember-learn/ember-website: 98 findings — all from<EsCard>(ember-styleguide's addon component, renders<li>) used as a list item inside<ul>. With the fix, those EPC reports drop entirely.hashicorp/design-system: ~25 internal classic addon list-item components, same pattern at varying scales.Test plan
test/glint-fixtures/node_modules/fake-card-addon/ships a real.hbstemplate at the canonical addon path; consumer imports from the package and uses inside<ul>componentTagMapresolves<FakeCard>→'li'npm test— 128/128 passingnpm run typecheck:tests— cleannpm run build— cleanCompanion
Stacks naturally with #16 (
fix/fp-toc-satisfies-element) and #18 (fix/fp-yielded-curried-component) — together they cover the four flavours of root-tag resolution: class form, TOC satisfies form, TOC annotation form / yielded curried, and now classic addon.hbs.