Preserve barrel side effects when rewriting deep imports#21352
Merged
NullVoxPopuli merged 1 commit intoemberjs:nvp/remove-barrel-importsfrom May 1, 2026
Merged
Conversation
The smoke tests on emberjs#21350 fail with "Cannot read properties of null (reading 'syscall')" in `AppendOpcodes.evaluate`. Cause: the lint autofix replaced barrel imports of `@glimmer/runtime` with deep paths (`@glimmer/runtime/lib/...`), so nothing pulled in the package index. The index's `import './lib/bootstrap';` was the only thing loading the opcode handler files (each `lib/compiled/opcodes/*.ts` registers via top-level `APPEND_OPCODES.add(...)`), so 28 of 90 handlers stayed unregistered. Fix: own the bootstrap from the file that actually consumes registered handlers — `lib/vm/low-level.ts`, where `evaluateSyscall` calls `APPEND_OPCODES.evaluate(...)`. Any deep-import path that uses the VM (directly or transitively) now triggers bootstrap; consumers that don't need the VM don't pay for opcode handlers in their bundle. Verified by counting top-level `APPEND_OPCODES.add(...)` calls in `dist/dev`: origin/main: 90 in a chunk loaded by everyone nvp/remove-barrel-imports: 62 loaded + 28 trapped in the unused `@glimmer/runtime/index.js` this commit: 90 reachable from `@ember/-internals/glimmer/index.js` Note for reviewers: the validator duplicate-package guard in `@glimmer/validator/index.ts` and the `class EmberObject extends CoreObject.extend(Observable)` extension in `@ember/object/index.ts` are also barrel-only side effects. EmberObject's case is handled naturally — `import EmberObject from '@ember/object'` is a default import, which the lint rule's `kept` mechanism preserves on the barrel. The validator guard becomes dormant under deep imports; if that's a concern, move the registration check into a shared leaf module (e.g. `lib/meta.ts`) in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
NullVoxPopuli-ai-agent
force-pushed
the
nvp/fix-barrel-imports-lint
branch
from
e36a604 to
fb0e583
Compare
NullVoxPopuli
merged commit cfe606e
into
emberjs:nvp/remove-barrel-imports
40 checks passed
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.
Summary
Fixes the failing smoke tests on #21350 with a 5-line change in
@glimmer/runtime. No lint-rule changes.The failure (
Cannot read properties of null (reading 'syscall')inAppendOpcodes.evaluate) was a real correctness issue — not a flake. After the lint autofix replaced barrel imports with deep imports, nothing imported@glimmer/runtime/index.tsanymore, and that index'simport './lib/bootstrap';was the only thing loading the opcode handler files. Eachlib/compiled/opcodes/*.tsregisters handlers via top-levelAPPEND_OPCODES.add(...), so when the bootstrap chain stopped, 28 of 90 handlers stayed unregistered — opcodes those handlers cover ({{on}}, dynamic helpers, etc.) hitnull.syscallat evaluation.Approach
Own the bootstrap from the file that actually consumes registered handlers —
lib/vm/low-level.ts, whereevaluateSyscallcallsAPPEND_OPCODES.evaluate(...). Any deep-import path that uses the VM (directly or transitively) now triggers bootstrap; consumers that don't need the VM don't pay for opcode handlers in their bundle. Removed the equivalent side-effect import from the package barrel since it's now redundant.I considered solving this in the lint rule (detect side-effect imports in barrels and re-emit
import 'BARREL';to preserve them) — pushed up an earlier version of this PR doing exactly that. But adding bare side-effect imports of whole barrels back undoes the whole tree-shaking point of #21350. Better to put the trigger where the demand actually is.Verified
I built
dist/devfrom each version and counted top-levelAPPEND_OPCODES.add(...)calls in chunks that are actually loaded:nvp/remove-barrel-imports@glimmer/runtime/index.jsThe 90 handlers in this PR's
dist/devare reachable from@ember/-internals/glimmer/index.js→setup-registry-...js→render-...js(28 adds, plus a side-effectimport './untouchable-this-...js'for the other 62).Note for reviewers
Two other barrel-only side effects in this PR's diff that might be worth a follow-up:
@glimmer/validator/index.tshas aglobalThis-based duplicate-package guard. It becomes dormant under deep imports — silent corruption is now possible if two copies ship. Move the registration check into a shared leaf module (e.g.lib/meta.ts) to keep it firing.@ember/object/index.tsrunsclass EmberObject extends CoreObject.extend(Observable)at module load. This one's actually fine:import EmberObject from '@ember/object'is a default import, which the rule'skeptmechanism preserves on the barrel. Anyone usingEmberObjectstill triggers the extension.Test plan
pnpm lintcleanpnpm build:jssucceedsAPPEND_OPCODES.add(...)calls reachable from@ember/-internals/glimmer/index.jsindist/devpnpm test:nodepassespnpm type-check:internalsreports same 10 pre-existing errors as the base branch🤖 Generated with Claude Code