Erase as |…| block-param clause when Glint substitutes a block-form component#14
Open
Erase as |…| block-param clause when Glint substitutes a block-form component#14
as |…| block-param clause when Glint substitutes a block-form component#14Conversation
…ram clause from the renamed open tag.
There was a problem hiding this comment.
Pull request overview
Fixes a false-positive cascade in html-validate output when Glint substitutes a yielding, block-form component invocation (e.g. <MyList as |item|>…</MyList>) to a native tag by ensuring the Glimmer-only block-param clause is removed from the emitted open tag.
Changes:
- In block-form Glint substitution, detect and blank the last
as |…|clause in the open-tag source span whennode.blockParamsare present. - Add unit tests asserting that
|…|block-param syntax does not leak into the blanked output for substituted block-form components.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
blank.ts |
Blanks the `as |
test/blank.test.ts |
Adds regression tests to ensure block-param syntax is removed when substituting block-form components to native tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review on #14: the test name claimed coverage for comma-separated and typed block-param forms but the src only exercised the space-separated form (which is what the AST actually carries — typed annotations are stripped earlier by stripBlockParamTypeAnnotations, and Glimmer's parser doesn't accept the comma form anyway). Adjusted the name and comment to describe what's tested.
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
When Glint resolves a yielding component to a native tag (e.g.
<MyList as |item|>→<div>), the in-place open-tag rename inhandleGlintSubstitution's block-form path renamed<MyList>→<div>but left theas |item|clause in place. html-validate's parser then treated|item|as an attribute, firingattr-case(Attribute "|item|" should be lowercase); downstream rules likeelement-permitted-contentcascaded.The rendered DOM has no such attribute — block params are a Glimmer-side binding for yielded content, not HTML — so the blanker must erase the
as |…|clause when it does the in-place rename.Implementation
In the block-form Glint substitution path, when
node.blockParams.length > 0, scan the open-tag text for\bas\s+\|[^|]*\|and add the LAST match toblankRanges. The last match is unambiguously the block-params clause (per Glimmer syntax it's always the rightmost thing before>), defending against the rare case where an attribute value happens to contain a literalas |x|.Surfaced by
Ecosystem CI on
hashicorp/design-system: 881× spuriousattr-caseplus a cascade ofelement-permitted-contentandelement-required-attributesreports — all rooted in this single leak. After the fix HDS dropped from 1184 → 303 findings (with Glint enabled).Test plan
<MyList as |item|>substituted to<div>—|item|must not appear in outputas |item index|covered by the same regexnpm test— 129/129 passingnpm run typecheck:tests— cleannpm run build— clean