fix(kit): try resolving module subpaths in resolveNuxtModule fn
#32386
Conversation
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses module resolution for Nuxt by attempting to resolve subpaths when the original module name isn’t found.
- Modified the iteration over module paths to allow for in-loop mutations.
- Introduced a while loop to iteratively shorten the module path until a match is found.
Comments suppressed due to low confidence (1)
packages/kit/src/resolve.ts:115
- Consider adding a comment explaining the purpose of iteratively shortening the module subpath. This will improve code understandability for future maintainers.
while (lastIndexOfPathInResolvedPath === -1 && path.includes('/')) {
packages/kit/src/resolve.ts
Outdated
| @@ -104,12 +104,22 @@ export async function resolveNuxtModule (base: string, paths: string[]): Promise | |||
| const resolved: string[] = [] | |||
| const resolver = createResolver(base) | |||
|
|
|||
| for (const path of paths) { | |||
| for (let path of paths) { | |||
There was a problem hiding this comment.
Consider using a separate temporary variable for the modified path instead of reassigning the loop variable. This can help prevent confusion and ensure that the original path value remains intact for any future reference.
Copilot generated this review using guidance from copilot-instructions.md.
WalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/kit/src/resolve.ts (2)
107-110: Avoid hard-coded/index.ts– use path utilities for robustnessSplitting on the literal string
'/index.ts'will mis-behave if:
- the path ends with
index.mjs/index.js- the file separator is
\(Windows)index.tsappears in the middle of the stringUsing
pathe.dirname()(already imported) or a regexp keeps the logic portable and future-proof.- resolved.push(path.split('/index.ts')[0]!) + // Works for any extension and OS path separator + if (path.endsWith('.ts')) { + resolved.push(dirname(path)) + } else { + resolved.push(path) + }
120-122: Possible duplicate entries in the result arrayWhen multiple input paths resolve to the same directory (e.g.
foo,foo/index.ts), the same string can bepushed repeatedly.
If the consumer expects uniqueness, wrapresolvedin aSetor deduplicate before returning.- const resolved: string[] = [] + const resolved = new Set<string>() … - resolved.push(resolvedPath.slice(0, lastIndexOfPathInResolvedPath + path.length)) + resolved.add(resolvedPath.slice(0, lastIndexOfPathInResolvedPath + path.length)) … - return resolved + return Array.from(resolved)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/kit/src/resolve.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: codeql (actions)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (1)
packages/kit/src/resolve.ts (1)
113-118: Path comparison is OS-sensitive – verify behaviour on Windows
String#lastIndexOfworks on raw strings; WindowsresolvedPathwill contain back-slashes (\) whereaspathuses forward-slashes. The loop will then never find a match.If cross-platform support is required, normalise both to POSIX (
pathe.toUnix) or to the platform separator before comparing.
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #32386 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/kit/src/resolve.test.ts(2 hunks)packages/kit/src/resolve.ts(2 hunks)packages/kit/src/template.ts(2 hunks)packages/nuxt/src/core/nitro.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kit/src/resolve.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/core/nitro.tspackages/kit/src/template.tspackages/kit/src/resolve.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/kit/src/resolve.test.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write unit tests for core functionality using `vitest`
packages/nuxt/src/core/nitro.ts (5)
Learnt from: GalacticHypernova
PR: #26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In packages/nuxt/src/components/plugins/loader.ts, the references to resolve and distDir are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: huang-julien
PR: #29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In packages/nuxt/src/app/components/nuxt-root.vue, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : nullinstead of wrapping the import in a computed property or importing the component unconditionally.
Learnt from: GalacticHypernova
PR: #29661
File: packages/kit/src/template.ts:227-229
Timestamp: 2024-11-28T21:22:40.496Z
Learning: In packages/kit/src/template.ts, when updating the EXTENSION_RE regular expression for TypeScript configuration, avoid using patterns like (\.\w+)+$ as they can result in catastrophic backtracking.
Learnt from: TheAlexLichter
PR: #31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using rolldownVersion (not rollupVersion) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Learnt from: Tofandel
PR: nuxt/nuxt#0
File: :0-0
Timestamp: 2024-11-11T12:34:22.648Z
Learning: Ensure that AI-generated summaries accurately reflect the key changes in the PR, focusing on notable changes such as the removal of unused imports and variables starting with underscores.
packages/kit/src/template.ts (5)
Learnt from: GalacticHypernova
PR: #29661
File: packages/kit/src/template.ts:227-229
Timestamp: 2024-11-28T21:22:40.496Z
Learning: In packages/kit/src/template.ts, when updating the EXTENSION_RE regular expression for TypeScript configuration, avoid using patterns like (\.\w+)+$ as they can result in catastrophic backtracking.
Learnt from: GalacticHypernova
PR: #26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In packages/nuxt/src/components/plugins/loader.ts, the references to resolve and distDir are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: huang-julien
PR: #29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In packages/nuxt/src/app/components/nuxt-root.vue, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : nullinstead of wrapping the import in a computed property or importing the component unconditionally.
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.{ts,tsx} : Follow standard TypeScript conventions and best practices
Learnt from: Tofandel
PR: nuxt/nuxt#0
File: :0-0
Timestamp: 2024-11-11T12:34:22.648Z
Learning: Ensure that AI-generated summaries accurately reflect the key changes in the PR, focusing on notable changes such as the removal of unused imports and variables starting with underscores.
packages/kit/src/resolve.test.ts (5)
Learnt from: GalacticHypernova
PR: #26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In packages/nuxt/src/components/plugins/loader.ts, the references to resolve and distDir are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write unit tests for core functionality using vitest
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to /e2e//*.{ts,js} : Write end-to-end tests using Playwright and @nuxt/test-utils
Learnt from: GalacticHypernova
PR: #29661
File: packages/kit/src/template.ts:227-229
Timestamp: 2024-11-28T21:22:40.496Z
Learning: In packages/kit/src/template.ts, when updating the EXTENSION_RE regular expression for TypeScript configuration, avoid using patterns like (\.\w+)+$ as they can result in catastrophic backtracking.
Learnt from: huang-julien
PR: #29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In packages/nuxt/src/app/components/nuxt-root.vue, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : nullinstead of wrapping the import in a computed property or importing the component unconditionally.
🧬 Code Graph Analysis (2)
packages/nuxt/src/core/nitro.ts (1)
packages/kit/src/resolve.ts (1)
resolveNuxtModule(104-124)
packages/kit/src/resolve.test.ts (1)
packages/kit/src/resolve.ts (1)
resolveNuxtModule(104-124)
⏰ 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: build
- GitHub Check: code
- GitHub Check: lint-docs
🔇 Additional comments (8)
packages/nuxt/src/core/nitro.ts (2)
49-57: LGTM! Enhanced module entry path collection.The prioritisation of
m.meta?.rawPathoverm.entryPathprovides more accurate module path information, which is essential for resolving module subpaths correctly. This change aligns with the PR objective to fix module subpath resolution issues.
152-158: LGTM! Improved TypeScript include coverage for modules.The expansion to include both
runtime/serveranddist/runtime/serverpaths ensures comprehensive TypeScript coverage for different module structures and build outputs. The use offlatMapefficiently creates the required flat array structure.packages/kit/src/template.ts (3)
276-280: LGTM! Consistent module entry path collection.The logic mirrors the improvements in
packages/nuxt/src/core/nitro.ts, prioritisingm.meta?.rawPathoverm.entryPathfor more accurate module path information. The use ofgetDirectory(path)appropriately normalises paths to directories.
286-286: LGTM! Improved module path filtering.The additional
path.startsWith(rootDirWithSlash)condition appropriately narrows the scope to modules within the root directory, preventing external dependencies from being processed inappropriately in this context.
301-301: LGTM! Logical exclusion of dist files.The addition of
dist/*.*to the exclusion list appropriately prevents build artifacts from being included in type generation, maintaining consistency with the selective inclusion approach used elsewhere in the codebase.packages/kit/src/resolve.test.ts (3)
1-1: LGTM! Appropriate test imports.The new imports support the
resolveNuxtModuletest functionality:statfor directory validation,withTrailingSlashfor consistent path formatting, andresolveNuxtModulefor the function under test.Also applies to: 4-4, 6-6
32-44: LGTM! Comprehensive test structure.The test provides excellent coverage of the
resolveNuxtModulefunction with appropriate use of inline snapshots for verification and clear assertions on the expected behaviour. The combination of installed modules plus an additional test module ensures thorough testing of the resolution logic.
46-49: LGTM! Thorough path validation.The validation logic properly verifies that resolved paths are directories and end with the expected 'node_modules/' suffix. The error handling with
.catch(() => false)ensures robust testing behaviour.
Co-authored-by: Daniel Roe <daniel@roe.dev>
🔗 Linked issue
resolves #32225
📚 Description
As much as there might be a better way to solve this, that's what I came up with so far - trying to resolve with subpaths one by one if original module name is not found.
Module in
nuxt.config:@nuxt/test-utils/moduleOriginal resolved path:
<...>/node_modules/.pnpm/@nuxt+test-utils@3.19.1_@pl_14bfbf51f08bfde6e72d5192217ed19c/node_modules/@nuxt/test-utils/dist/module.mjsTransformed path returned from
resolveNuxtModule:<...>/node_modules/.pnpm/@nuxt+test-utils@3.19.1_@pl_14bfbf51f08bfde6e72d5192217ed19c/node_modules/@nuxt/test-utils