refactor(vite): drop externality and use vite internal config
#30634
Conversation
|
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
WalkthroughThe pull request introduces significant changes to module handling in the Vite and Nuxt integration, specifically focusing on external module resolution. The primary modifications involve removing the The implementation now uses a more direct method of handling module externalization, with updated import statements and modified dependency configuration. The 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (1)
packages/vite/src/vite-node.ts (1)
119-126: Review the broad inclusion of modules in 'deps.inline'.The regular expressions added to
deps.inlinemay inline more modules than necessary:
/^virtual:/matches all modules starting withvirtual:./\.ts$/matches all files ending with.ts./^#/matches all modules starting with#./\?/matches any module containing?.Inlining all TypeScript files and modules containing
?might impact performance. Consider refining these patterns to target only the required modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/vite/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (2)
packages/vite/src/utils/external.ts(0 hunks)packages/vite/src/vite-node.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/vite/src/utils/external.ts
🔇 Additional comments (3)
packages/vite/src/vite-node.ts (3)
9-9: Import of 'withTrailingSlash' is appropriate.The addition of
withTrailingSlashfrom the'ufo'module appears correct, as it is utilised later in constructing paths with trailing slashes.
11-11: Usage of 'escapeStringRegexp' enhances security.Employing
escapeStringRegexpto escape paths when creating regular expressions helps prevent RegExp injection vulnerabilities.
128-129: Verify the constructed regular expression for external dependencies.The regular expression created for
deps.externalaims to match the shared directory:new RegExp('^' + escapeStringRegexp(withTrailingSlash(resolve(ctx.nuxt.options.rootDir, ctx.nuxt.options.dir.shared))))Ensure that:
ctx.nuxt.options.dir.sharedaccurately reflects the shared directory path.- The resolved path correctly targets the intended modules without excluding necessary files or including unintended ones.
To confirm the regular expression matches the intended files, you can run the following script:
✅ Verification successful
The regular expression construction for external dependencies is correctly implemented
The implementation follows Nuxt's established patterns and safely handles path resolution by:
- Using proper escaping via
escape-string-regexp- Ensuring consistent path matching with
withTrailingSlash- Correctly resolving paths relative to the root directory
This pattern is consistently used across the codebase, particularly in both
vite-node.tsandserver.ts, confirming its reliability for matching shared directory paths.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List files matching the external dependencies regex. regex="^$(echo "$(node -e "console.log(require('path').resolve('${ctx.nuxt.options.rootDir}', '${ctx.nuxt.options.dir.shared}'))")/")" regex=$(echo "$regex" | sed 's/[\/&]/\\&/g') echo "Constructed regex: $regex" echo "Files matching the regex:" find . -type f | grep -E "$regex"Length of output: 412
Script:
#!/bin/bash # Search for shared directory configuration and similar patterns echo "=== Searching for shared directory configuration ===" rg -l "dir.shared" -g "!*.{lock,md}" echo -e "\n=== Looking for similar regex constructions ===" rg "new RegExp.*escapeStringRegexp.*resolve" -g "!*.{lock,md}" echo -e "\n=== Checking path resolution patterns ===" ast-grep --pattern 'resolve($_, $dir.shared)'Length of output: 993
Script:
#!/bin/bash # Examine the shared directory configuration echo "=== Checking shared directory schema ===" rg -A 5 "shared.*:" packages/schema/src/config/common.ts echo -e "\n=== Checking for imports of the regex construction ===" rg "escapeStringRegexp.*from|withTrailingSlash.*from" packages/vite/src/vite-node.tsLength of output: 963
| /^#/, | ||
| ...transpile({ isServer: true, isDev: ctx.nuxt.options.dev }), |
There was a problem hiding this comment.
we can drop this as it's already set on ssr.noExternal here:
nuxt/packages/vite/src/server.ts
Lines 67 to 73 in a01c41b
and this seems to be respected by vite-node when creating the server:
| external: [ | ||
| '#shared', | ||
| new RegExp('^' + escapeStringRegexp(withTrailingSlash(resolve(ctx.nuxt.options.rootDir, ctx.nuxt.options.dir.shared)))), |
There was a problem hiding this comment.
I added some files in the shared/utils/ directory to use them in app/, and it seems that this part of the code might be causing an error where .ts files cannot be recognized:
Unknown file extension ".ts" for /Users/.../anything.tsThere was a problem hiding this comment.
could you replicate in the basic fixture and push a pr with a failing test? 🙏
There was a problem hiding this comment.
Ooops! I added a test in the basic fixture, and everything worked well—I couldn’t replicate the issue. However, I did encounter an error with parsing .ts files under the playground/ folder.
at __node_internal_captureLargerStackTrace (node:internal/errors:497:5)
at new NodeError (node:internal/errors:406:5)
at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:99:9)
at defaultGetFormat (node:internal/modules/esm/get_format:142:36)
at defaultLoad (node:internal/modules/esm/load:120:20)
at ModuleLoader.load (node:internal/modules/esm/loader:396:13)
at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:278:56)
at new ModuleJob (node:internal/modules/esm/module_job:65:26)
at #createModuleJob (node:internal/modules/esm/loader:290:17)
at ModuleLoader.getJobFromResolveResult (node:internal/modules/esm/loader:248:34)
at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:229:17)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async ModuleLoader.import (node:internal/modules/esm/loader:315:23)Since nuxt-nightly hasn’t been updated to this version yet, I’ll test it again once the update is available.
There was a problem hiding this comment.
I tried reproducing this issue using StackBlitz with Nuxt version 4.0.0-28957560.b6143461, and the error can be reproduced. Would it be helpful to open an issue for easier tracking?
👉 https://stackblitz.com/edit/nuxt-starter-f1lj7peg?file=nuxt.config.ts
🔗 Linked issue
📚 Description
this drops
externalitywhich should clean up the dependency tree a bit (graph).this will need testing to ensure behaviour is still the same in dev - you can use the packages published in this comment to do so.