Repository navigation
chore: declare sideEffects: false and guard the claim in CI - #40
Merged
Merged
Conversation
publint suggested the field. The measurement says it saves a consumer nothing: fixtures bundled under rollup 4.65, vite 7.3, webpack 5.112 and esbuild 0.25, against two tarballs differing only in this field, gave a zero byte delta for all three entry points, minified and not, for import and require consumers alike. splitting: false emits each entry as one self-contained module and sideEffects is module-granularity, so there is nothing for it to drop. The harness detects a delta where one exists: a bare unused import goes from 51,355 bytes to 30. The field therefore goes in because it is true and currently missing, not to save bytes, which makes the guard the substance of this change. Nothing verified the claim before: publint and attw both ignore the field, and every test imports from src/, so the built artifacts' import behaviour was never exercised at all. The guard imports each published entry in a child process with the browser globals, console and the stream writers trapped, and fails on any access. Module-scope reads fail as well as writes, because environment detection at import time breaks the SSR-safety the README claims. Results come back through a file rather than the child's stdout, since a module that logs while evaluating would corrupt them, and that is exactly the module this check exists to catch. The probe self-tests against a generated impure module on every run: a check that cannot fail is worse than no check.
A module that started a timer during evaluation kept the probe's child process alive, and spawnSync had no timeout, so the parent blocked until the CI job's own limit - six hours by default, with publish.yml's gate stuck behind it. A side effect this check exists to catch could stall the pipeline rather than fail it. Exiting the child after it writes its result would have cured the hang by turning that case into a silent pass, which is worse. So the child now compares process.getActiveResourcesInfo() either side of the import and reports anything new, then exits. That covers the class generically rather than by enumerating APIs: a timer reports as Timeout, a listening socket as TCPServerWrap, without either being trapped by name. spawnSync keeps a 30s timeout as a backstop for a handle the counter cannot see. The self-test gains timer fixtures in both module formats, so the detector cannot quietly stop working. Also widens the import-failure hint, which blamed a missing build for what is equally often a missing dependency.
The reason given was false. `typeof window !== 'undefined'` under the probe evaluates to false and never throws, and a module caching that on a server still returns the empty values the README promises, so it does not break SSR-safety. What throws on a server is a module-scope property read such as `window.location.search`, and the trap sees both as "read window". So the rule stands on the fact that the trap cannot tell those apart, which makes failing every read the conservative choice, and on the house invariant: all 13 guard sites in src/ sit inside function bodies, and __tests__/setup.ts re-stubs those globals per test, so a cached module-level read would make the suite order-dependent. A wrong reason is what invites someone to conclude the rule itself is wrong. Also drops the clauses that only restated the script header and the commit message, and corrects "saves no bytes", which the measurement contradicts for a consumer importing nothing at all: that case drops 51,355 bytes to 30.
The guard passed, at exit 0, a built entry that mutated process.env, patched Array.prototype, added a globalThis property and registered a process exit listener. Trapping twelve browser globals plus console and the stream writers covers what this library touches today and nothing else, so it licensed sideEffects: false for a module doing observable work. No enumeration closes that, because the set of things a module can mutate is open. So the probe now snapshots the environment either side of the import - globalThis own properties, process.env entries, the prototypes of Object, Array, String, Number, Function, Promise and Date, and the active resource counts - and reports anything that changed. One pass covers globalThis writes, Object.defineProperty on globalThis, prototype patching and env mutation together. Named traps are added only where a diff cannot see the call at all: the schedulers and the process hooks. Two channels close the remaining gaps. Any child stdout is now itself a side effect, since results travel by file and only the module under test can write there - which catches fs.writeSync(1, ...) bypassing the stream trap. And an entry importing a node builtin fails outright: synchronous work like fs.writeFileSync leaves nothing to observe, so the bundle shape is the only place to rule it out. No entry imports one today. A failing import no longer discards what was recorded before it failed, and the stale-build hint is given only for a module-resolution error, which is what it actually diagnoses. collectEntries now takes every ./ target that is not a declaration or a map and fails on an extension it cannot load, instead of silently checking a subset: an exports map using .mjs for the ESM entry previously left the entry a bundler tree-shakes unchecked while reporting success. The self-test grew to one fixture per detector, so a detector that stops working fails the run rather than the claim.
The probe replaces process.stderr.write before importing, so Node's own module-loader debug lines went through the trap and were reported as the package's side effects: with NODE_DEBUG=module set, four of six entries failed on an unmodified build, blaming the package for the debugger's output. A guard that fails on a clean tree gets switched off by whoever it blocks, which costs more than the class it detects. The child's env drops NODE_DEBUG. A genuine process.stderr.write at import is still caught with NODE_DEBUG=module set, so the detector is unchanged; only Node's own diagnostics stop being attributed to the package.
…e seen The diff compared property names, so only additions showed up. A built entry that reassigned Array.prototype.includes, replaced String.prototype.trim and globalThis.structuredClone, deleted Array.prototype.at and called process.chdir passed at exit 0 - verified, with a marker proving those lines ran. Replacing an existing method is the common polyfill shape, so the rewrite closed the rarer half of the class it was aimed at. Globals and prototypes now record an identity token per key, taken from the property descriptor so an accessor's getter is never invoked - the browser traps are accessors, and reading them would report a side effect against the module under test. The diff is symmetric, so addition, replacement and removal all surface, and Object.isFrozen is recorded alongside. process.cwd and process.title are snapshotted too. Adding a negative control to the self-test immediately earned it: a module with no side effects at all was reported as having three. Comparing with === made the NaN global read as replaced on every run, and reading descriptors materialises Node's undici dispatcher, which registers symbol-keyed globals with an incrementing counter, so each snapshot saw new ones. Object.is fixes the first; globals are now compared over string keys only, which costs the detection of a symbol-keyed global and is recorded as a known gap. The builtin check now resolves specifiers against module.builtinModules rather than a hand-written list, and anchors on import() as well, so node:fs/promises and dynamic imports are no longer missed. collectEntries skips wildcard subpaths and package.json, which would otherwise abort the run as unloadable. Scheduler stubs return a Timeout-shaped handle so a module that unrefs one is not also reported as unimportable, and a spawn error now reaches the message.
…claim Three claims were wrong or misleading. "mutations to statics rather than prototypes are not snapshotted" implied prototypes were covered. Only the seven in WATCHED_PROTOTYPES are: Map.prototype.x and Error.prototype.toJSON both pass clean, as does any process state beyond cwd and title. The gap list now says so, and loses the unverifiable "in rough order of likelihood". The externals comment pointed at tsup.config.ts as the source of truth. That config externalises only react, while this list needs react/jsx-runtime too, because tsup matches a prefix and this script requires each specifier literally - and tsup additionally externalises every dependency and peerDependency without the config naming them. A maintainer following the old comment would have missed a new runtime dependency. It now states the invariant the list actually has to satisfy. CLAUDE.md claimed a cached module-level read would make the suite order-dependent. It would not: vitest evaluates a test file's imports at collection, before any beforeEach, so such a read captures jsdom's original every time and fails deterministically in any order. The throw-on-a-server reason already carried the rule, so the second rationale is gone, along with a brittle count of guard sites - one of the 13 was in a markdown file - and a copy of reasoning the script header already holds. Also: the probe exits for the parent's 30s timeout rather than stalling it indefinitely; the undici symbols carry a version suffix, not a counter; the self-test directive now excludes the builtin check, which runs in the parent; and NODE_DEBUG_NATIVE is no longer deleted, since its output is C++-side and never reaches the trapped writer.
On Node 20.0-20.17 and 22.0-22.9 a clean build reported `dist/react/index.js: evaluated on import — read window`. Reproduced on 20.14.0. Nothing in the package does it: the react entry is ESM importing a CJS dependency, and the first such import in a process initialises Node's cjs-module-lexer, which reads `window` on those minors. The read lands after the traps go up, so it is attributed to the entry that triggered it. Warming the lexer first does not fix it. Importing a throwaway .cjs through the ESM loader before taking the baseline still failed on 20.14.0 and added a CloseReq handle that lands after the baseline, so it traded one false positive for two. The check now states the requirement and exits instead. A precondition a reader can act on is worth more than a side-effect claim that is not true, and CI is unaffected: the workflows pin major versions, which float to the latest minor. Development below Node 22 is already out of reach, since jsdom needs 22+. Also qualifies the undici note as Node 22+, where it was measured, and adds the setter-only accessor to the gap list.
jackmisner
added
documentation
Merged
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.
Why, and the case against doing this
publint suggests
"sideEffects": false. Measured, it saves a consumer zero bytes. Two tarballs differing only in that field, consumer fixtures bundled for production:../server./reactSame with the minifier off (webpack, root entry: 51,370 → 51,370) and for
require()consumers. The harness detects a delta where one exists — a bare unused import drops 51,355 bytes to 30 — so these are measurements, not a broken rig.The reason is structural:
splitting: falseemits each entry as one self-contained module, andsideEffectsis module-granularity. The module holdingtoSnakeCaseis used, so it is included either way. The flag only pays off when none of an entry's exports are used.So the field goes in because it is true and was missing, not to save bytes — and that makes the guard the substance of this PR. A one-line consumer-facing claim that nothing verifies is how it quietly becomes false later.
What changed
"sideEffects": falseinpackage.jsonscripts/assert-no-import-side-effects.js, appended tocheck:package, so it runs in CI'spackage-checkjob and inpublish.yml's publish gateCLAUDE.mdGotchasThe guard imports every entry in
package.json'sexportsin a child process and fails if evaluation was observable. Detection is layered, because no single mechanism covers the ways module evaluation can be seen:console, the stream writers, the four schedulers and fiveprocesshooks.globalThis,process.env, the prototypes of Object/Array/String/Number/Function/Promise/Date (withObject.isFrozen),process.cwd/title, and active handle counts. Symmetric, so addition, replacement and removal surface.fs.writeSync(1, …)bypassing the stream trap.module.builtinModules. Synchronousfswork leaves nothing observable, so the bundle shape is the only place to rule it out.How it was verified
The guard must fail on breakage, so every case below is a deliberate break of a real built entry, reverted after. All caught, each with a specific message:
process.envmutation ·Array.prototypeaddition ·Array.prototype.includesreplacement ·delete Array.prototype.at·Object.freeze(Date.prototype)·globalThisaddition ·globalThis.structuredClonereplacement ·Object.defineProperty(globalThis, …)·process.chdir·process.on·queueMicrotask·setTimeout·setIntervalleaving a handle ·require('node:timers').setInterval(caught only by the handle diff) ·require('node:fs/promises')·fs.writeSync(1, …)· a guardedtypeof windowread · a directprocess.stderr.writePlus controls: a clean build exits 0; a missing
dist/namesnpm run build; and disabling a detector makes the self-test fail with a precise reason. The self-test runs 18 fixtures on every invocation — one per probe detector, plus a negative control asserting a pure module reports nothing.Cross-platform: clean and byte-identical on Node 20, 22, 24 and 26, on macOS and in official
node:Docker images, 12–15 repeated runs per version. Guard cost 0.5–1.0 s.What the review found
Three separate false-negative classes, each verified before fixing and each in the class the previous iteration was meant to close:
process.exit(0)alone would have cured the hang by making it a silent pass, so the child now diffs active resources and reports what it left behind.process.env, patchingArray.prototype, adding aglobalThisproperty and registering aprocess.on('exit')listener passed at exit 0.And one false positive, caught by the negative control within seconds of it being added: a module with no side effects was reported as having three.
NaN !== NaNmade theNaNglobal read as replaced every run, and reading descriptors materialises Node's undici dispatcher, which registers symbol-keyed globals. That is the argument for a negative control in one incident — the guard's credibility rests as much on not firing as on firing.Corrections to my own prose, all from review: the SSR justification for failing module-scope reads was false (
typeof windowdoesn't break SSR;window.locationdoes, and the trap can't tell them apart); the externals comment namedtsup.config.tsas a source of truth it isn't; a claim that a cached module-level read makes the test suite order-dependent was wrong (vitest evaluates imports beforebeforeEach, so it fails deterministically); and the byte measurement above was removed fromCLAUDE.mdbecause it isn't reproducible from the repo — hence its presence here.Known gaps, stated rather than implied
Synchronous filesystem work leaves nothing to observe (the builtin check covers that class at the bundle level). An unhandled rejection at module scope needs the event loop to turn. The snapshot does not reach statics, prototypes outside the watched seven, process state beyond
cwd/title, symbol-keyedglobalThisadditions, or a setter-only accessor swap.The guard refuses to run on Node 20.0–20.17 and 22.0–22.9: those minors read
windowwhile initialising the CJS module lexer, which the react entry triggers, and it would be reported against the package. Reproduced on 20.14.0. Warming the lexer first makes it worse. CI pins major versions, which float to the latest minor.Review status
Three reviewers over the first commit, two over the rewrite, and a claim audit over the final diff. The last commit (
1beeba1, the Node precondition) post-dates the review round — it is ~20 lines, verified on both 20.14.0 and 24.18.1.