🍕 Node globals: real diagnostics + opt-in fix, generalized server-boundary warning - #260
Merged
Merged
Conversation
|
Coverage is 83.02% — jordan/node-globals-diagnostics into master. No base build found for |
Needed by the opt-in nodeGlobals.Buffer feature landing in the next commit: @rollup/plugin-inject auto-imports a free identifier only into modules that reference it; buffer is the real npm polyfill package, used only when nodeGlobals.Buffer is explicitly set to 'polyfill' (byte-capable use, e.g. createHmac) rather than the default minimal browser-compat.js stub. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ndary warning
Three related additions to the Vite pipeline's handling of things that leak
from server-only code into a browser-reachable bundle. All are either
report-only or default-off — no behavior change for any site that doesn't
configure them.
1. A working diagnostic for bare Node-global references (scanBareNodeGlobals,
viteBareNodeGlobalsPlugin)
claycli.config.js's Vite pipeline has no equivalent of Browserify's
insert-module-globals, so `Buffer.from(...)` written as a bare identifier
(no `require('buffer')`/`import 'buffer'`) throws `Buffer is not defined`
at runtime with zero build-time signal -- browser-compat.js's stub only
intercepts an explicit import of the built-in.
The obvious fix -- stop suppressing Rollup's MISSING_GLOBAL_NAME warning --
turns out not to work. Verified empirically (a throwaway fixture built
standalone with vite.build()): MISSING_GLOBAL_NAME is only ever emitted for
an external dependency with no `output.globals` entry on a umd/iife build.
This pipeline always builds `format: 'esm'` with no externals, so Rollup
silently leaves the free identifier untouched -- no warning fires, for any
input. The suppression is still removed and the MISSING_GLOBAL_NAME
collection path kept (harmless, forward-compatible for a future iife/umd
entry), but it is not what makes the diagnostic real.
scanBareNodeGlobals() is: a `transform` hook that pattern-matches source
text for `Buffer` used as a free identifier, skipping node_modules/virtual
module ids and any file that already imports 'buffer' explicitly (already
handled by browser-compat.js). Deliberately scoped to `Buffer` only, not
`process`/`global` -- both of those are already substituted at build time
by buildDefines() for their common legitimate forms (process.env.*,
process.browser, global -> globalThis), so scanning for them would be
mostly false positives. A regex, not an AST walk -- it can't distinguish a
free identifier from a property name (`{ Buffer: 1 }`), an accepted,
documented trade-off for not adding a parser dependency.
All occurrences aggregate into one end-of-build summary (naming every hit
and the file it's in), printed once after both Rollup passes resolve.
Verified manually end-to-end: a fixture component with a bare
`Buffer.from(...)` call is correctly reported with its file path; a file
that explicitly `require('buffer')`s first is not flagged; a clean file
produces no output at all.
2. Opt-in auto-injection (nodeGlobals.Buffer)
`bundlerConfig().nodeGlobals.Buffer = true` adds @rollup/plugin-inject,
configured to auto-import `Buffer` from 'buffer' ONLY into modules that
reference the free identifier -- resolving through the existing
browser-compat.js stub, which already prefers a real globalThis.Buffer
when present. `nodeGlobals.Buffer = 'polyfill'` instead aliases the
'buffer' specifier to the real npm package (added in the previous commit)
for byte-capable use (e.g. createHmac, which the minimal stub can't
support). Unset (default): no plugin added, zero bytes, zero behavior
change -- verified manually by comparing bundle output size/content
between an unset build and one with the fixture's Buffer reference (1081
bytes, no stub present) against the enabled build (1581 bytes, stub
inlined, `Buffer2.from(...)` correctly rewritten from the bare reference).
3. Generalized server-boundary warning (serverOnlyPackages)
The existing services/server -> services/client rewrite in
service-rewrite.js only covers Clay's own isomorphic service convention --
it has no opinion on a third-party package (Amphora, an Amphora plugin)
that is itself server-only and gets transitively pulled into a
browser-reachable module graph. `bundlerConfig().serverOnlyPackages`
(default []) is a package-path-prefix allowlist that warns (never blocks --
this is a stopgap diagnostic, not a hard guarantee like the services/server
rewrite) when a matching import is resolved. Threaded in as a constructor
argument to viteServiceRewritePlugin(), matching every other Vite plugin in
this directory (none of them read claycli.config.js themselves --
buildPlugins() resolves config once and passes it in); an earlier version
of this had the plugin call getConfigValue() directly, which was both
inconsistent with that convention and read from the wrong config
mechanism entirely (the legacy top-level claycli.config.js key rather than
the bundlerConfig() hook every other Vite-specific option uses) -- caught
and fixed during manual verification, not a released regression.
Verified manually: a configured prefix warns with the importing file named;
an unconfigured (default []) site is a no-op, verified via both the direct
plugin call and a full build.
`npm test` (lint + all 447 existing tests) is green. No test files added or
modified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`npx eslint lib cli index.js` -- the full lint command `npm test` actually runs -- currently fails on master: normalizeRequestedSteps has a cyclomatic complexity of 10 (max 8), introduced in #259 and missed there because that PR's own verification only linted the specific files it touched, not the full repo command. Splits the function into splitOnlyEntries() (CSV/repeated-flag parsing) and classifyOnlyEntries() (recognized vs. unrecognized step names), leaving normalizeRequestedSteps as a thin orchestrator (complexity 7). No behavior change -- reverified the exact case #259 added: an unrecognized --only value still throws with the same message. Discovered while rebasing this branch onto master post-#259; fixing it here so this branch's own CI is green. Also opening a standalone fix directly against master, since master itself fails `npm test` right now independent of this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jjpaulino
force-pushed
the
jordan/node-globals-diagnostics
branch
from
September 15, 2026 18:54
4c4b586 to
f1005fe
Compare
jjpaulino
added a commit
that referenced
this pull request
Sep 15, 2026
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.
TL;DR — for reviewers short on time
clay vitehas no equivalent of Browserify'sinsert-module-globals, so a bareBuffer.from(...)reference (no accompanyingrequire('buffer')) throwsReferenceError: Buffer is not definedin the browser with zero build-time signal —browser-compat.js's stub only intercepts an explicit import of the built-in. This PR adds: (1) a real build-time diagnostic naming every file with a bareBufferreference, (2) an opt-innodeGlobals.Bufferconfig that fixes it, and (3) a generalizedserverOnlyPackageswarning for third-party server-only packages (e.g. Amphora internals) leaking into a browser bundle. All three are report-only or default-off — no behavior change for any site that doesn't configure them.nodeGlobals.BufferandserverOnlyPackagesboth default to off/empty — verified manually that an unset build produces byte-identical output to before this PR (compared bundle size/content of a fixture with and without the flag). Fully reversible.npm test(lint + all 447 existing tests) green. Extensively verified manually end-to-end using a throwaway fixture project with a real bare-Bufferreference: (a) default build — silent failure reproduced (the exact reported bug shape), now caught by the new scan and named in a build summary; (b)nodeGlobals.Buffer: true— the bare reference is correctly rewritten to resolve through the existing browser-compat stub (confirmed in the actual bundle output); (c)nodeGlobals.Bufferunset — bundle size/content unchanged; (d) a file that alreadyrequire('buffer')s explicitly is correctly NOT flagged; (e)serverOnlyPackageswarns when configured (viabundlerConfig()) and is a confirmed no-op when unset, both via direct plugin invocation and a full build. No test files added or modified.lib/cmd/vite/scripts.jsL1764–L1790 —scanBareNodeGlobals()/viteBareNodeGlobalsPlugin(), the real diagnostic. The originally-obvious fix (stop suppressing Rollup'sMISSING_GLOBAL_NAMEwarning) turned out not to work: verified empirically that Rollup only ever emits it for an external dependency missing anoutput.globalsentry on aumd/iifebuild, and this pipeline always buildsformat: 'esm'— so that warning silently never fires, for any input, on this pipeline. This is a regex-based source scan instead (deliberatelyBuffer-only, notprocess/global— see the code comment for why those would be mostly false positives against whatbuildDefines()already substitutes).lib/cmd/vite/scripts.jsL1764–L1790 — the bare-Buffer scan and its always-on plugin (report-only).lib/cmd/vite/scripts.jsL387–L408 —buildPlugins(): the scan plugin is always added;@rollup/plugin-injectis added only whennodeGlobals.Bufferis truthy, auto-importingBufferonly into modules that reference the free identifier.lib/cmd/vite/scripts.jsL534–L546 —buildResolveAlias():nodeGlobals.Buffer === 'polyfill'aliases'buffer'to the real npm package (for byte-capable use likecreateHmac, which the minimal stub can't support) instead of the default stub.lib/cmd/vite/plugins/service-rewrite.jsL84–L113 —serverOnlyPackages: a config-driven, default-empty package-path-prefix allowlist that warns (never blocks) when a matching import resolves. Threaded in as a constructor argument viabundlerConfig(), matching every other Vite plugin in this directory (none of them readclaycli.config.jsdirectly —buildPlugins()resolves config once and passes it in).lib/cmd/vite/scripts.jsL191–L194 — newnodeGlobals/serverOnlyPackagesdefaults ongetViteConfig()(both empty/off).package.json/package-lock.json— adds@rollup/plugin-injectandbufferas dependencies (the latter used only fornodeGlobals.Buffer: 'polyfill').Feature Info
Description
Gives
Buffer is not defined-class bugs a real build-time diagnostic and a fix, and generalizes the existing services/server→services/client boundary check to cover third-party server-only packages too. Everything here is additive/opt-in; nothing changes for an unconfigured site.Validation Points
serverOnlyPackagesinitially readclaycli.config.jsdirectly via the legacygetConfigValue()path (inconsistent with every other Vite plugin's dependency-injection convention, and a different config mechanism thannodeGlobalsuses) — caught and refactored during manual verification, not a released regression.Bufferscan is a regex heuristic, not an AST walk: it can't distinguish a free identifier from a property name (e.g.{ Buffer: 1 }) — an accepted, documented trade-off to avoid a parser dependency for a report-only diagnostic.🤖 Generated with Claude Code