Add experimental externals detection #2083
Conversation
🦋 Changeset detectedLatest commit: 3ff8366 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change introduces an experimental configuration option, Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/cli-v3/src/build/braces.d.ts (1)
84-88: Fix typo in JSDoc –quanitifiers➜quantifiersSmall documentation typo – makes copy-paste searches harder and looks unpolished.
- * In regular expressions, quanitifiers can be used to specify how many times a token can be repeated. + * In regular expressions, quantifiers can be used to specify how many times a token can be repeated.packages/cli-v3/src/build/externals.ts (2)
565-568: Broaden non-bare import detection
isBareModuleImport()misses other common prefixes (node:,https:,http:,npm:,#for import maps).
Including them avoids unnecessary resolution attempts and false positives.- const excludes = [".", "/", "~", "file:", "data:"]; + const excludes = [ + ".", "/", "~", + "file:", "data:", "http:", "https:", + "node:", "npm:", "#" + ];
570-572: Handle sub-path built-in imports
builtinModulescontains plain module names ('fs','url').
Imports likenode:fs/promisesorfs/promiseswon’t match and will be mis-classified.-function isBuiltinModule(path: string): boolean { - return builtinModules.includes(path.replace("node:", "")); +function isBuiltinModule(path: string): boolean { + const cleaned = path.replace(/^node:/, ""); + return builtinModules.some((m) => cleaned === m || cleaned.startsWith(`${m}/`)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreferences/v3-catalog/package.jsonis excluded by!references/**references/v3-catalog/src/trigger/binaries.tsis excluded by!references/**references/v3-catalog/trigger.config.tsis excluded by!references/**
📒 Files selected for processing (6)
.changeset/grumpy-wasps-fold.md(1 hunks)packages/cli-v3/package.json(1 hunks)packages/cli-v3/src/build/braces.d.ts(1 hunks)packages/cli-v3/src/build/externals.ts(5 hunks)packages/cli-v3/src/commands/deploy.ts(2 hunks)packages/core/src/v3/config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/core/src/v3/config.ts (1)
173-184: Well-documented experimental feature.The configuration property is properly documented with clear warning notices, default value, and purpose explanation. The JSDoc comments help users understand when and why they should use this experimental feature.
packages/cli-v3/package.json (1)
99-99:✅ Verification successful
Verify braces dependency version is appropriate.
The
bracespackage is added as a new dependency, which is necessary for the experimental external detection feature.
🏁 Script executed:
#!/bin/bash # Check if the braces package has any known security vulnerabilities and latest version # Get the latest version npm view braces version # Check for any security advisories npm audit --package braces@3.0.3 --json | jq '.metadata.vulnerabilities'Length of output: 458
🏁 Script executed:
#!/bin/bash # Check for existing lockfiles to enable vulnerability auditing find . -type f -name 'package-lock.json' find . -type f -name 'yarn.lock'Length of output: 711
🏁 Script executed:
#!/bin/bash # Fetch npm security advisories for the braces package curl -s "https://registry.npmjs.org/-/npm/v1/security/advisories?module=braces" \ | jq '.objects[] | {id, title, vulnerable_versions, severity, url}'Length of output: 207
Dependency Validation: braces@3.0.3 is up-to-date and has no known advisories
I confirmed via
npm view braces versionthat 3.0.3 is the latest release and found no security advisories for this package in the npm registry. The addition ofbracesfor the experimental external detection feature can proceed as is..changeset/grumpy-wasps-fold.md (1)
1-7: Appropriate changeset entry for experimental feature.The changeset correctly identifies this as a patch version update and concisely describes the new experimental feature. This aligns with semantic versioning principles since it's adding a new capability without breaking existing functionality.
packages/cli-v3/src/commands/deploy.ts (2)
2-2: New import for improved error handling.The addition of
tryCatchutility from the core package supports the improved error handling implementation.
241-267: Improved error handling with explicit try-catch pattern.The refactoring from direct
awaitto a structured error-handling approach usingtryCatchis a significant improvement. This pattern:
- Provides explicit error handling for the build process
- Ensures the spinner is properly stopped with a clear failure message
- Adds better visibility into build completion via the
onBundleCompletecallbackThis change enhances both developer experience and code reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli-v3/src/build/externals.ts (1)
268-406: Well-structured implementation of the autoDetectExternal feature.The implementation properly:
- Only runs when explicitly enabled via the autoDetectExternal config flag
- Filters out non-package imports early
- Uses caching to avoid redundant work
- Has comprehensive detection logic for native modules
Since this is an experimental feature, consider adding a warning log when it's enabled to remind users about its status.
if (resolvedConfig.build?.autoDetectExternal) { + logger.warn("[externals] Using experimental autoDetectExternal feature. This may cause unexpected behavior."); build.onResolve(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/cli-v3/src/build/braces.d.ts(1 hunks)packages/cli-v3/src/build/externals.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/cli-v3/src/build/braces.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/cli-v3/src/build/externals.ts (6)
147-150: Effective caching implementation for performance optimization.The caching strategy using two separate maps is a good approach to improve performance by avoiding repeated file system operations and package.json parsing. This is especially important for large projects with many dependencies.
163-169: Enhanced logging will help with troubleshooting.The expanded logging now includes details about externals collection, cache sizes, and the auto-detection status, which will be valuable for diagnosing issues in this experimental feature.
368-370: Correct implementation for native file extension detection.This implementation correctly uses
resolvedPathinstead ofargs.pathwhen checking for native file extensions, addressing a previous issue identified in code review.
383-388: Comprehensive detection of native files.The regex pattern covers common native file extensions, and the check against expanded file entries is a good approach to detect potential native dependencies.
570-577: The module detection utility functions look correct.Both
isBareModuleImportandisBuiltinModulefunctions are implemented correctly and serve their purpose well. The excludes list for bare module imports is comprehensive.
623-652: Well-implemented caching for package.json resolution.The
findNearestPackageJsonfunction effectively uses the cache and properly handles errors during resolution. This will significantly improve performance when processing many modules from the same package.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cli-v3/src/build/externals.ts (1)
579-621: Add test coverage forisMainPackageJsonNo existing tests reference this function, so dedicated unit tests should be added to validate its behavior across edge cases.
🧹 Nitpick comments (5)
packages/cli-v3/src/build/externals.ts (5)
268-280: Consider adding error handling for configuration validationThe experimental flag check is correctly implemented, but consider adding validation to ensure users understand the implications of enabling this experimental feature.
if (resolvedConfig.build?.experimental_autoDetectExternal) { + logger.debug("[externals] Using experimental auto-detection of externals. This may cause build or runtime issues."); build.onResolve(
288-298: Ensure module resolution errors are properly categorizedThe error handling for module resolution is good, but consider distinguishing between different types of resolution errors to provide more specific guidance to users.
if (resolveError) { logger.debug("[externals][auto] Resolve module error", { path: args.path, resolveError, + resolveErrorCode: resolveError instanceof Error ? (resolveError as any).code : 'unknown', + resolveErrorMessage: resolveError instanceof Error ? resolveError.message : String(resolveError) }); return; }
579-621: Consider additional edge cases in isMainPackageJsonThe function distinguishes between main package.json files and type marker files using a hardcoded list. Consider adding handling for additional fields that may appear in marker files.
const markerFields = new Set([ "type", "sideEffects", "browser", "main", "module", "react-native", "name", + "exports", + "types", + "typings" ]);
613-617: Improve error logging verbosityThe error logging could be more informative by including the file path in the error message.
if ("code" in error && error.code !== "ENOENT") { logger.debug("[externals][containsEsmTypeMarkers] Error", { + filePath, error: error.message, }); }
639-644: Consider refactoring to use async/await for clarityWhile
tryCatchworks correctly, consider using async/await with standard try/catch for consistency with other parts of the codebase and improved readability.- const [error, packageJsonPath] = await tryCatch( - resolvePackageJSON(dirname(basePath), { - test: isMainPackageJson, - }) - ); - - if (error) { - cache.set(baseDir, null); - return null; - } + try { + const packageJsonPath = await resolvePackageJSON(dirname(basePath), { + test: isMainPackageJson, + }); + + cache.set(baseDir, packageJsonPath); + return packageJsonPath; + } catch (error) { + cache.set(baseDir, null); + return null; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
references/v3-catalog/trigger.config.tsis excluded by!references/**
📒 Files selected for processing (3)
.changeset/grumpy-wasps-fold.md(1 hunks)packages/cli-v3/src/build/externals.ts(5 hunks)packages/core/src/v3/config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/grumpy-wasps-fold.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/v3/config.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/cli-v3/src/build/externals.ts (2)
147-150: Good practice using caching for performance optimizationThe implementation uses two caching mechanisms to avoid redundant filesystem operations, which is a good performance optimization especially for the build process that might process thousands of modules.
163-169: Improved debugging with comprehensive loggingThe enhanced logging will make it easier to diagnose issues with the automatic external detection feature. Including both the flag status and cache sizes provides valuable context.
If your tasks fail to build you can try the new
build.autoDetectExternaltrigger config option. It should NOT be used otherwise as it may break deploys, we're still testing this.