Skip to content

Don't remap scope locals in non-reference positions - #120

Draft
NullVoxPopuli wants to merge 2 commits into
object-scope-test-coveragefrom
no-remap-object-keys
Draft

Don't remap scope locals in non-reference positions#120
NullVoxPopuli wants to merge 2 commits into
object-scope-test-coveragefrom
no-remap-object-keys

Conversation

@NullVoxPopuli

@NullVoxPopuli NullVoxPopuli commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Based on #121, which adds the wireScope helper this asserts with. Extracted from #117 so that PR can stay purely about literal scope values.

remapAndBindIdentifiers rewrites a scope local's hbs name to its JS name throughout the template compiler's output, and it did so for every matching identifier — including ones that aren't references.

That's fine while the wire format's scope is an array (scope: () => [Setup]), because a local's name only ever appears in reference position. When the compiler emits the scope as an object — ember-source 7 does — the name also appears as a property key, and renaming it there drops the name the template refers to:

// input
precompileTemplate('<Foo />', { strictMode: true, scope: () => ({ Foo: Setup }) })

// before
scope: () => ({ Setup })

// after
scope: () => ({ Foo: Setup })

The fix is to skip identifiers babel doesn't consider references (path.isReferencedIdentifier()), which also covers the other non-reference positions an identifier can occupy.

Commits

The first commit is intentionally red. It adds the test on its own, and because the bug only exists where the compiler emits keys, the failure lands in the try scenarios that run ember-source 7 — ember-latest and ember-beta, 7 legs in all, while every ember ≤6 leg passes:

AssertionError: expected { Setup: 'Setup' } to deeply equal { Foo: 'Setup' }

The second commit is the seven-line fix.

Worth noting: package.json pins ember-source at ^6.4.0-beta.1, so the primary Tests job doesn't exercise the object-form scope at all. Bumping that baseline to ^7 would put this coverage in the main job — the try scenarios already pin 3.28/4.12/5.12/6.4 explicitly, so nothing would lose coverage. Left out of this PR as a separate call.

🤖 Generated with Claude Code

NullVoxPopuli and others added 2 commits August 12, 2026 14:29
`remapAndBindIdentifiers` rewrites a scope local's hbs name to its JS name
throughout the compiler's output, including in positions that aren't
references. Where the compiler emits an object-form scope, that renames the
property key too, so `{ Foo: Setup }` compiles to `scope: () => ({ Setup })`
and the name the template refers to is lost.

This passes against a compiler that emits the array-form scope, which has no
keys to lose, and fails against one that emits the object form — currently
ember-source 7, which the ember-latest and ember-beta try scenarios cover:

    expected { Setup: 'Setup' } to deeply equal { Foo: 'Setup' }

The fix is in the next commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`remapAndBindIdentifiers` rewrites a scope local's hbs name to its JS name
throughout the template compiler's output, and it did so for every matching
identifier, including ones that aren't references.

Skip identifiers babel doesn't consider references, which fixes the property
key case from the previous commit and covers the other non-reference positions
an identifier can occupy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant