-
-
Notifications
You must be signed in to change notification settings - Fork 772
feat: automatically trace known native pkgs #3923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request moves the nf3 dependency from devDependencies to dependencies and integrates nf3-based dependency tracing into the build system. Changes include importing NodeNativePackages from nf3, computing traced dependencies that combine library packages with user options, and adding tracing instrumentation with detailed logging to the externals plugin. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/build/plugins.ts (1)
60-74: Consider escaping package names in the RegExp to prevent potential ReDoS.While
NodeNativePackagescontains safe static values,nitro.options.traceDepsis user-provided configuration. If a package name contains regex metacharacters (e.g.,+,*,(), it could cause unexpected matching behavior or, in edge cases, ReDoS.The
escapeRegExputility from../../utils/regex.tsis already used inexternals.tsfor this purpose.🔎 Suggested fix
+import { escapeRegExp } from "../utils/regex.ts";Then update the RegExp construction:
const traceDeps = [ ...new Set([...NodeNativePackages, ...(nitro.options.traceDeps || [])]), ]; + const escapedDeps = traceDeps.map(escapeRegExp); plugins.push( externals({ rootDir: nitro.options.rootDir, conditions: nitro.options.exportConditions || ["default"], exclude: [...base.noExternal], include: isDevOrPrerender ? undefined : [ new RegExp( - `^(?:${traceDeps.join("|")})|[/\\\\]node_modules[/\\\\](?:${traceDeps.join("|")})(?:[/\\\\])` + `^(?:${escapedDeps.join("|")})|[/\\\\]node_modules[/\\\\](?:${escapedDeps.join("|")})(?:[/\\\\])` ), ],
📜 Review details
Configuration used: Organization 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.jsonsrc/build/plugins.tssrc/build/plugins/externals.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js,tsx,jsx}: Usepathefor cross-platform path operations instead of Node.jsnode:path
Use ESM and modern JavaScript
Do not add comments explaining what the line does unless prompted
Files:
src/build/plugins.tssrc/build/plugins/externals.ts
src/{build,dev,runner,cli}/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
consolafor logging in build/dev code; usenitro.loggerwhen available
Files:
src/build/plugins.tssrc/build/plugins/externals.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
unstoragefor storage abstraction
Files:
src/build/plugins.tssrc/build/plugins/externals.ts
src/build/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Virtual modules must be registered in
src/build/virtual.ts
Files:
src/build/plugins.tssrc/build/plugins/externals.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Avoid introducing new dependencies unless strictly necessary; add to `devDependencies` unless required in runtime logic
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/{build,dev,runner,cli}/**/*.{ts,js} : Use `consola` for logging in build/dev code; use `nitro.logger` when available
Applied to files:
src/build/plugins.ts
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Avoid introducing new dependencies unless strictly necessary; add to `devDependencies` unless required in runtime logic
Applied to files:
src/build/plugins.tspackage.json
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/runtime/**/*.{ts,js} : Keep runtime code minimal and side-effect free to reduce bundle size
Applied to files:
src/build/plugins/externals.ts
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/build/**/*.{ts,js} : Virtual modules must be registered in `src/build/virtual.ts`
Applied to files:
src/build/plugins/externals.ts
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/**/*.{ts,js,tsx,jsx} : Use `pathe` for cross-platform path operations instead of Node.js `node:path`
Applied to files:
src/build/plugins/externals.ts
🧬 Code graph analysis (1)
src/build/plugins.ts (1)
src/build/plugins/externals.ts (1)
externals(27-181)
🪛 ast-grep (0.40.3)
src/build/plugins.ts
[warning] 70-72: 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(
^(?:${traceDeps.join("|")})|[/\\\\]node_modules[/\\\\](?:${traceDeps.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 (4)
package.json (1)
68-68: LGTM on moving nf3 to dependencies.Since Nitro is distributed as a library and the build code (using
NodeNativePackagesandtraceNodeModules) runs in the consumer's context, nf3 correctly belongs independenciesrather thandevDependencies. The optional peer dependency pattern (lines 170, 177-179) allows consumers to override the version if needed.src/build/plugins.ts (1)
15-15: LGTM on the nf3 import.Importing
NodeNativePackagesdirectly provides access to the known native packages database for automatic tracing.src/build/plugins/externals.ts (2)
10-10: LGTM on using consola for logging.This follows the coding guideline to use
consolafor logging in build/dev code.
146-177: Well-structured tracing instrumentation.The implementation follows good practices:
- Dynamic import of
nf3avoids loading it when tracing is disabled- Timing measurement provides useful build performance insights
- The hooks capture traced file/package counts before the final summary
- Helpful info message about OS/architecture matching for native modules
This PR enables automatically tracing known dependencies with native modules (nf3 db)
Adding back nf3 to main deps as tracing is an important feature.