refactor!: overhaul externals support #3799
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
📝 WalkthroughWalkthroughReplaces previous Rollup-based node externals tracing with a new custom Rollup externals plugin, introduces path-aware regex helpers, expands NitroOptions with flexible noExternals/traceDeps and wasm defaults, removes several example/preset externals inlines, and adds dynamic dependency import/install utilities and related test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/presets/node.test.ts (1)
3-3: Remove commented-out import instead of leaving dead code.The
isWindowsimport is no longer used after removing the Windows-specific skip condition. Consider removing it entirely rather than commenting it out.-// import { isWindows } from "std-env";src/build/plugins/externals.ts (1)
64-70: Minor: Redundant prefix check.The
id.includes(":")check handles existing prefixed builtins, butbuiltinModulesfrom Node.js only contains unprefixed names (e.g., "fs", not "node:fs"). The ternary will always take the false branch (addingnode:). This is harmless but the conditional could be simplified.if (builtinModules.includes(id)) { return { resolvedBy: PLUGIN_NAME, external: true, - id: id.includes(":") ? id : `node:${id}`, + id: `node:${id}`, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
examples/shiki/nitro.config.ts(0 hunks)examples/vite-ssr-solid/vite.config.mjs(0 hunks)examples/vite-ssr-vue-router/vite.config.mjs(0 hunks)package.json(1 hunks)src/build/config.ts(3 hunks)src/build/plugins.ts(2 hunks)src/build/plugins/externals.ts(1 hunks)src/build/plugins/virtual.ts(2 hunks)src/build/vite/env.ts(2 hunks)src/config/defaults.ts(2 hunks)src/presets/cloudflare/dev.ts(0 hunks)src/presets/deno/preset.ts(0 hunks)src/runtime/internal/vite/dev-worker.mjs(1 hunks)src/runtime/meta.ts(1 hunks)src/types/config.ts(2 hunks)src/utils/regex.ts(1 hunks)test/examples.test.ts(1 hunks)test/fixture/nitro.config.ts(1 hunks)test/fixture/server/routes/modules.ts(0 hunks)test/presets/node.test.ts(2 hunks)test/presets/vercel.test.ts(2 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- src/presets/cloudflare/dev.ts
- examples/shiki/nitro.config.ts
- examples/vite-ssr-solid/vite.config.mjs
- src/presets/deno/preset.ts
- test/fixture/server/routes/modules.ts
- examples/vite-ssr-vue-router/vite.config.mjs
🧰 Additional context used
🧬 Code graph analysis (5)
src/build/vite/env.ts (1)
src/runtime/meta.ts (1)
runtimeDependencies(13-31)
src/build/plugins/virtual.ts (1)
src/utils/regex.ts (1)
pathRegExp(7-16)
src/build/plugins/externals.ts (2)
src/utils/regex.ts (2)
toPathRegExp(18-26)escapeRegExp(3-5)src/runtime/meta.ts (1)
pkgDir(11-11)
src/build/config.ts (2)
src/utils/regex.ts (2)
pathRegExp(7-16)toPathRegExp(18-26)src/runtime/meta.ts (1)
pkgDir(11-11)
src/build/plugins.ts (1)
src/build/plugins/externals.ts (1)
externals(26-154)
🪛 ast-grep (0.40.0)
src/build/vite/env.ts
[warning] 42-42: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(${runtimeDependencies.join("|")})$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
src/build/plugins/virtual.ts
[warning] 30-32: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^(${[...modules.keys()].map((id) => pathRegExp(id)).join("|")})$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (23)
src/runtime/internal/vite/dev-worker.mjs (1)
41-41: LGTM!The change from
DEBUGtoNITRO_DEBUGprovides better namespace isolation for Nitro-specific debugging, avoiding conflicts with other tools that use the genericDEBUGenvironment variable.test/presets/vercel.test.ts (1)
530-533: LGTM!Moving the
vercel.jsonfile creation into abeforeAllhook is a good practice for test setup, ensuring proper sequencing of test initialization.src/runtime/meta.ts (1)
11-11: LGTM!Reordering the
pkgDirexport is a non-functional change that doesn't impact the public API.src/build/plugins/virtual.ts (1)
2-2: LGTM! Windows path compatibility improvement.Replacing
escapeRegExpwithpathRegExpadds Windows path separator handling (converting backslashes to a pattern that matches both/and\), which is essential for cross-platform compatibility.Regarding the static analysis warning about ReDoS: while constructing regex from variables can be risky, in this case:
- The module IDs are controlled by the build system, not user input
pathRegExpalready appliesescapeRegExpinternally, neutralizing special regex characters- The risk of malicious input is minimal in this build-time context
Also applies to: 32-32
src/utils/regex.ts (2)
7-16: LGTM! Windows-aware path escaping is correctly implemented.The
pathRegExpfunction properly handles cross-platform path matching by:
- Normalizing Windows backslashes to forward slashes before escaping
- After escaping special regex characters, converting slashes to
[/\\]on Windows to match both separatorsThis ensures path patterns work correctly regardless of the platform or path separator format.
18-26: LGTM! Good input validation with informative error handling.The
toPathRegExpfunction provides a clean API that:
- Preserves existing
RegExpobjects unchanged- Converts strings to path-aware regex patterns
- Includes proper type checking with a descriptive
TypeErrorthat includes the invalid input as the causeThe error handling approach using
{ cause: input }is a modern best practice for debugging.test/examples.test.ts (1)
21-21: Ensure the vite-ssr-tss-react example is properly configured for production builds.The
vite-ssr-tss-reactexample directory exists with appropriate Vite and package configuration. However, verify that the example's production build and tests pass by running the test suite, as configuration inspection alone cannot confirm test execution status.vitest.config.ts (1)
3-4: No action needed. SettingNODE_ENVto"production"invitest.config.tsdoes not break tests. Analysis shows no test files depend onNODE_ENVbeing unset or set to"test"or"development". The few tests that referenceprocess.envonly interact with custom environment variables (e.g.,NITRO_A,NITRO_BUILDER,MAIL_HOST), notNODE_ENV. The production mode configuration is intentional for this server framework.package.json (1)
159-159: No action required for unwasm package version.Version
0.5.2of theunwasmpackage exists and is the latest release on npm with no known security vulnerabilities reported.src/config/defaults.ts (1)
36-36: LGTM!The new
wasmandtraceDepsdefaults are correctly initialized as empty objects/arrays, aligning with the updatedNitroOptionstype surface.Also applies to: 75-75
test/presets/node.test.ts (1)
36-41: LGTM!The simplified test correctly validates externals tracing by checking for the traced dependency file. The removal of platform-specific gating aligns with the unified externals handling introduced in this PR.
test/fixture/nitro.config.ts (1)
53-53: LGTM!The
traceDeps: ["@fixture"]configuration correctly enables dependency tracing for the fixture package, which is verified by the updated test intest/presets/node.test.ts.src/build/vite/env.ts (2)
42-44: Static analysis false positive: ReDoS warning is not applicable here.The
runtimeDependenciesarray imported fromnitro/metacontains hardcoded, known-safe package names (e.g., "crossws", "h3", "defu"). Since these are controlled literals without user input or special regex characters, there is no ReDoS risk. The pattern^(name1|name2|...)$is safe and efficient.
77-79: LGTM!The service environment correctly delegates to the base
noExternalconfiguration in dev mode while using standalone bundling for production, consistent with the nitro environment pattern.src/build/plugins.ts (1)
59-74: LGTM!The externals plugin integration is well-structured:
- Correctly gated on
nodemode withnoExternals !== true- Dev/prerender mode appropriately disables tracing for faster iteration
- Production builds trace dependencies to the server output directory
- The
exclude/includepatterns properly map to base config andtraceDepssrc/build/config.ts (1)
59-86: LGTM!The
getNoExternalshelper consolidates noExternal pattern computation cleanly:
- Correctly bundles TypeScript files, virtual modules, and the Nitro package directory
- Conditionally includes
.wasmfiles when wasm is enabled- Properly transforms user-provided
noExternalspatterns viatoPathRegExp- Sorting by regex source length is a reasonable optimization for pattern matching order
src/types/config.ts (1)
244-245: LGTM!The updated
noExternalsand newtraceDepstype definitions provide a flexible API:
noExternalsaccepts either a boolean to disable externalization entirely or an array of patterns to excludetraceDepsaccepts patterns to specify which dependencies to traceBoth being optional maintains backward compatibility.
src/build/plugins/externals.ts (6)
11-22: LGTM!The
ExternalsOptionstype is well-defined with appropriate field types. Thetraceoption correctly omits fields that are computed internally (rootDir,exportConditions,traceOptions).
31-46: LGTM!The filter logic correctly:
- Excludes virtual modules (
\0), internal references (#~.), protocol imports, and query-string imports- Requires matching at least one include pattern when specified
- Rejects any match against exclude patterns
133-152: LGTM!The
buildEndhandler correctly:
- Short-circuits when tracing is disabled or no paths were traced
- Dynamically imports
nf3to avoid bundling it when unused- Configures tracing with proper export conditions and root directory
206-218: LGTM with note on cache pattern.The
getPkgJSONcaching implementation works correctly. The cache-on-function pattern ((getPkgJSON as any)._cache) is functional but unconventional. A module-levelMaporWeakMapwould be more idiomatic, though this is a minor stylistic preference.
220-237: LGTM!The
flattenExportsutility correctly handles nested exports structures, distinguishing between subpaths (starting with.) and conditions. The recursive processing properly builds the flattened array of subpath/fsPath/condition triples.
163-173: ThetoImportfunction requires bothnameandsubpathto trace absolute paths, which may miss bare package root imports in edge cases.If
resolvedPathpoints to a package directory without a file extension (e.g.,/path/node_modules/lodash), bothtoImport(line 167) and theguessSubpathfallback (line 177) will returnundefinedbecause they requiresubpathto exist. In such cases, the module won't be traced. While standard module resolvers typically resolve to actual files (e.g.,index.js), a custom resolver or edge case could trigger this. Clarify whether this limitation is intentional, or consider handling bare package roots by returningjoin(name)whensubpathis missing.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(3 hunks)src/build/plugins/externals.ts(1 hunks)src/utils/dep.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/build/plugins/externals.ts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests-rolldown (windows-latest)
- GitHub Check: tests-rollup (windows-latest)
- GitHub Check: tests-rollup (ubuntu-latest)
🔇 Additional comments (6)
src/utils/dep.ts (6)
1-3: LGTM!The imports are appropriate for the dynamic dependency resolution and installation functionality.
5-12: LGTM!The function signature is well-structured with clear parameter names and appropriate generic typing.
38-42: LGTM!The error handling provides a clear, actionable message when installation is declined or unavailable.
44-50: LGTM!The installation process is well-implemented with good UX:
- Dynamic import of
nypmavoids unnecessary loading- Clear logging with timing information
- Uses
addDevDependencyappropriately
52-52: LGTM!The recursive retry with
_retry=trueensures the module is properly resolved after installation, with cache disabled for a fresh resolution attempt.
23-36: Remove the concern about thecancelparameter on line 34—it is correct.The syntax
cancel: "undefined"is a valid strategy for theconsola.promptAPI. When the prompt is canceled (e.g., via Ctrl+C), it resolves withundefined. This matches the consola documentation and is not a bug.Consider clarifying the comment on line 25 to mention both conditions.
The comment states "Do not install dependencies in test mode," but the condition also checks
_retry. For clarity:if (_retry || isTest) { - shouldInstall = false; // Do not install dependencies in test mode + shouldInstall = false; // Do not install dependencies in test mode or on retryLikely an incorrect or invalid review comment.
This PR changes how external dependencies are resolved in nitro v3.
In development mode a much faster implementation is used that leverages native builder resolution and filter hooks.
In production mode, external dependencies are now bundled by default unless added to
traceDeps.Opt-in tracing requires
nf3as dependency (an auto prompt will ask it) and is much faster with less resolution and better subpath guessing.