Add agadoo tree-shake regression check#21366
Merged
NullVoxPopuli merged 1 commit intoemberjs:nvp/configure-side-effectsfrom May 3, 2026
Merged
Conversation
cdfeb88 to
4de95e9
Compare
`sideEffects: false` declares a contract that's invisible to reviewers: any module-level mutation (`set*Manager(...)`, `Foo.reopenClass(...)`, `_backburner = new Backburner(...)`) silently rots the contract. Bundlers will drop these side effects when the symbol they ride along with isn't imported. Adds a `pnpm test:tree-shake` script that bundles each pure-today entry point with rollup as if a consumer imported it for side effects only, and asserts nothing survives. Regression in any known-pure entry fails CI with an actionable message. Today the list is 23 entries — `@glimmer/util`, `@glimmer/destroyable`, `@ember/owner`, `@ember/version`, etc. The 43 currently-impure entries (@ember/-internals/glimmer, @ember/runloop, @glimmer/runtime, …) are omitted because they have known top-level side effects that sideEffects: false promises bundlers can drop in practice anyway. Patch: agadoo's bundled acorn pins ecmaVersion 11 (ES2020), which chokes on private class fields and other ES2022+ syntax that appears in dist. The patch bumps it to `'latest'`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4de95e9 to
e8bb3bb
Compare
ade41e1
into
emberjs:nvp/configure-side-effects
77 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
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.
Stacks on top of #21365.
sideEffects: falseis a contract that's invisible to reviewers — any module-level mutation (set*Manager(...),Foo.reopenClass(...),_backburner = new Backburner(...)) silently rots the contract. Bundlers will drop these side effects when the symbol they ride along with isn't imported.This adds a
pnpm test:tree-shakescript that bundles each currently-pure entry point with rollup as if a consumer imported it for side effects only, and asserts nothing survives. Regression in any known-pure entry fails CI with an actionable message.What's checked
23 entries that are fully tree-shakeable today:
The 43 currently-impure entries (
@ember/-internals/glimmer,@ember/runloop,@glimmer/runtime, etc.) are omitted because they have known top-level side effects (manager registrations, validator state, backburner instance, etc.) thatsideEffects: falseis promising bundlers can drop in practice anyway.Files
bin/check-tree-shake.mjs— runsagadoo.check()on each known-pure entry, reports failurespackage.json— addsagadoodevDep andpnpm test:tree-shakescriptpatches/agadoo.patch— bumps agadoo's bundledacornfromecmaVersion: 11(ES2020) to'latest'so it doesn't choke on private class fields / other ES2022+ syntax that appears indist/Caveat
Agadoo bundles by absolute path with no nodeResolve plugin, so it doesn't actually read
ember-source/package.json'ssideEffects: false. That's a feature here, not a bug: it's measuring "is this file inherently pure?" not "does it tree-shake when consumed as a package." The contractsideEffects: falseis making is "no module gains a new top-level side effect that bundlers can't observe through their conservative heuristics," and agadoo enforces exactly that.Verification
Verified the failure mode by adding
globalThis.__OOPS__ = {}at the top level of@ember/destroyable/index.ts. The check correctly fails:Test plan
pnpm test:tree-shake— 23 entries verified pure🤖 Generated with Claude Code