Skip to content

Accept literal scope values produced by bundler constant inlining - #117

Open
NullVoxPopuli-ai-agent wants to merge 1 commit into
emberjs:no-remap-object-keysfrom
NullVoxPopuli-ai-agent:literal-scope-values
Open

Accept literal scope values produced by bundler constant inlining#117
NullVoxPopuli-ai-agent wants to merge 1 commit into
emberjs:no-remap-object-keysfrom
NullVoxPopuli-ai-agent:literal-scope-values

Conversation

@NullVoxPopuli-ai-agent

@NullVoxPopuli-ai-agent NullVoxPopuli-ai-agent commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #116

Based on #120 — please review that first; this PR is stacked on top of it.

A bundler's constant inlining — e.g. rolldown's default optimization.inlineConst — can legally rewrite

import { EMPTY } from './constants.js';
precompileTemplate('{{EMPTY}}', { strictMode: true, scope: () => ({ EMPTY }) });

into

precompileTemplate('{{EMPTY}}', { strictMode: true, scope: () => ({ EMPTY: "—" }) });

before this plugin ever sees the module (typically in a v2 addon published with targetFormat: 'hbs'), and parseScope then fails the consuming app's build with "Scope objects for precompileTemplate may only contain direct references to in-scope values … Found StringLiteral".

This is the literal-value analog of #14: rollup rewrote { FieldComponent } to { FieldComponent: HeadlessFormFieldComponent } and #17/#19 taught parseScope to accept renamed identifier references. A literal-valued scope property likewise still carries everything the compiler needs — the template-visible name and the value it resolves to.

Changes

  • parseScope accepts constant values: LiteralScopeValue is t.Literal minus regexes (a regex literal would construct a new stateful object per call) and template literals (they can interpolate arbitrary bindings), plus -/+-signed numbers, which are UnaryExpressions rather than literals. Anything else still errors, with the message extended to mention literals.
  • ScopeLocals mapping values are now ScopeValue = string | LiteralScopeValue, so an entry can carry the constant instead of a JS identifier name.
  • Wire format: remapAndBindIdentifiers replaces the local's identifier in the compiler output with the value, producing scope: () => ["—"].
  • targetFormat: 'hbs': buildScope re-emits the value in the rebuilt scope object, so the output round-trips through a later wire compile.
  • Unused literal entries are pruned from locals the same way identifier entries are.

Tests cover both target formats, pruning, the still-rejected CallExpression case, and the deliberately-excluded template-literal and regex shapes.

🤖 Generated with Claude Code

@NullVoxPopuli-ai-agent

Copy link
Copy Markdown
Contributor Author

Rebased onto main now that #118 is in. The rebase surfaced one real interaction: under Ember 7's object-form wire scope, the literal substitution in remapAndBindIdentifiers would have also replaced scope-object keys, producing invalid syntax like { -1: -1 }. Identifiers in non-computed key position are now skipped (keys keep the hbs name; the runtime reads values positionally), and shorthand is unset when a value is rewritten. Verified locally: full suite green on ember-source 3.28/6.4/7.1.0/7.2.0-beta.1.

Comment thread src/plugin.ts Outdated
// name
path.replaceWith(babel.types.identifier(scopeLocals.get(path.node.name)));
if (scopeLocals.has(path.node.name)) {
// Ember 7's wire format emits the scope as an object whose values are

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this here / is it important?

Comment thread src/plugin.ts
.map(([name, identifier]) =>
t.objectProperty(t.identifier(name), t.identifier(identifier), false, name !== 'this')
.map(([name, value]) =>
typeof value === 'string'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why string?

Comment thread src/expression-parser.ts Outdated

// the expression shapes a bundler's constant inlining can substitute for a
// reference to an imported constant
function isLiteralValue(node: t.ObjectProperty['value']): node is t.Expression {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type is too wide

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does babel have a type for all literal types

@ef4 ef4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing down the things we discussed on the embroider weekly meeting so I don't forget them.

Comment thread src/scope-locals.ts Outdated
// Each hbs name in the template's scope maps to either the name of a JS
// binding or, when a bundler's constant inlining has replaced the original
// binding with its statically-known value, a literal expression.
#mapping: Record<string, string | t.Expression> = {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either we support arbitrary expressions (in which case the separate string case is unnecessary) or we don't (in which case the t.Expression type is a lie). The implementation right now is half-way between the two.

Comment thread src/plugin.ts Outdated
// name
path.replaceWith(babel.types.identifier(scopeLocals.get(path.node.name)));
if (scopeLocals.has(path.node.name)) {
// Ember 7's wire format emits the scope as an object whose values are

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is concerning because this PR is supposed to have nothing to do with ember 7 support and because ember 7 support changes should be test-suite-only here, not effecting the actual implementation.

Comment thread src/expression-parser.ts Outdated

// the expression shapes a bundler's constant inlining can substitute for a
// reference to an imported constant
function isLiteralValue(node: t.ObjectProperty['value']): node is t.Expression {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type signatures doesn't match implementation. And implementation is probably not needed because babel already has type discrimination functions like this.

@NullVoxPopuli
NullVoxPopuli changed the base branch from main to no-remap-object-keys August 12, 2026 16:17
@NullVoxPopuli

Copy link
Copy Markdown
Contributor

Addressed the review feedback; rebased, so this now sits on top of #120.

@ef4's point about ember 7 in the implementation — you were right that it didn't belong here. Two separate things were bundled into that block:

  1. The object-key guard is a real bug that predates this PR. When the compiler emits the scope as an object, remapAndBindIdentifiers renames the property key along with the value, so { Foo: Setup } compiles to scope: () => ({ Setup }) and the name the template refers to is lost. That's now Don't remap scope locals in non-reference positions #120, framed as the general invariant (only references get remapped, via path.isReferencedIdentifier()) with a test that drives the plugin through a compilerPath mock emitting the object form — so it's pinned independently of which ember-source is installed. It fails on main.
  2. The shorthand = false calls were unnecessary. Babel's generator already prints the long form when the key and value diverge or the value isn't an identifier; output is byte-identical without them. Deleted.

With #120 as the base, this PR is literals-only again.

@ef4 on string | t.Expression being half-way — agreed, the type was a lie. It's now:

export type LiteralScopeValue =
  | Exclude<t.Literal, t.RegExpLiteral | t.TemplateLiteral>
  | SignedNumericLiteral;

export type ScopeValue = string | LiteralScopeValue;

We don't support arbitrary expressions, so the type now says exactly what we do support.

@NullVoxPopuli on "does babel have a type for all literal types" — yes, t.Literal, but it's slightly too wide for us: it includes RegExpLiteral (a regex literal would construct a new stateful object on every call, where the original was a shared constant) and TemplateLiteral (can interpolate expressions referencing bindings we know nothing about). Hence the Exclude. Going the other way, -1 is a UnaryExpression, not a literal at all, so SignedNumericLiteral covers that. The predicate is now built from t.isLiteral/t.isUnaryExpression rather than a hand-rolled switch, and returns the narrow type instead of t.Expression. Added tests for the two excluded shapes.

@NullVoxPopuli's "why string?" on buildScope — that falls out of the above: a scope entry is either the name of a JS binding (string) or a constant value (a node), and the two need different objectProperty construction.

Full suite green locally (133 tests).

A bundler's constant inlining (e.g. rolldown's default
optimization.inlineConst) can legally rewrite

  import { EMPTY } from './constants.js';
  precompileTemplate('{{EMPTY}}', { scope: () => ({ EMPTY }) })

into

  precompileTemplate('{{EMPTY}}', { scope: () => ({ EMPTY: "-" }) })

before this plugin ever sees the module, which parseScope rejected with
"may only contain direct references to in-scope values". This is the
literal-value analog of the renamed-identifier case fixed in 2.0.2: the
scope property still carries everything the compiler needs, the
template-visible name and the value it resolves to.

parseScope now accepts literal values (string, number, boolean, null,
bigint, and signed numbers), ScopeLocals carries them as expressions
alongside plain identifier names, wire-format output emits the literal
directly into the scope array, and hbs-format output re-emits it in the
rebuilt scope object.

Fixes emberjs#116

Co-Authored-By: Claude Fable 5 <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

4 participants