Conversation
|
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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). (2)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/nuxt/src/components/plugins/transform.ts (2)
137-137: Use a safer own-property check
Object.hasOwnis fine on Node 18+, but the canonical guard avoids potential polyfill issues and reads clearly in TS downlevel targets.- if (isObject(pkg) && isObject(pkg.imports) && Object.hasOwn(pkg.imports, '#components')) { + if (isObject(pkg) && isObject(pkg.imports) && Object.prototype.hasOwnProperty.call(pkg.imports, '#components')) {
116-147: Micro-cache package boundary checks to cut I/O in large reposWe may traverse and read the same package.json multiple times during a build. A tiny in-memory cache keyed by nearest containing dir will reduce repeated FS calls.
Below are minimal additions; happy to wire them if you prefer.
Add near the top of
TransformPlugin(inside the factory):// Cache: dir -> boolean (true means skip transform) const pkgImportsCache = new Map<string, boolean>()Then memoise around the traversal:
const cacheKey = dir const cached = pkgImportsCache.get(cacheKey) if (cached !== undefined) { if (cached) return } else { // after determining `hasInternalMapping`, store it: pkgImportsCache.set(cacheKey, /* boolean */ hasInternalMapping) }packages/nuxt/test/components-transform.test.ts (2)
28-36: Prefer explicit matcher over snapshot for undefinedAsserting
undefinedis clearer and avoids snapshot churn.- expect(code).toMatchInlineSnapshot(`undefined`) + expect(code).toBeUndefined()
34-36: Add a couple more edge-case tests
- ID with a querystring (e.g.
'/foo.vue?vue&type=script') to ensure the traversal still ignores internal#components.- Nearest package.json without
imports['#components']to confirm we still transform.I can add both tests using the existing fixture and a second fixture without the mapping—want me to push a follow-up patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/nuxt/test/package-fixture/package.jsonis excluded by!**/package.json
📒 Files selected for processing (3)
packages/nuxt/src/components/plugins/transform.ts(2 hunks)packages/nuxt/test/components-transform.test.ts(2 hunks)pnpm-workspace.yaml(1 hunks)
🧰 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/test/components-transform.test.tspackages/nuxt/src/components/plugins/transform.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/nuxt/test/components-transform.test.ts
🧠 Learnings (5)
📓 Common learnings
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: 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.
📚 Learning: 2025-07-18T16:46:07.446Z
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`
Applied to files:
pnpm-workspace.yaml
📚 Learning: 2025-07-18T16:46:07.446Z
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`
Applied to files:
packages/nuxt/test/components-transform.test.ts
📚 Learning: 2024-11-05T15:22:54.759Z
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.
Applied to files:
packages/nuxt/src/components/plugins/transform.ts
📚 Learning: 2024-12-12T12:36:34.871Z
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.
Applied to files:
packages/nuxt/src/components/plugins/transform.ts
🔇 Additional comments (2)
pnpm-workspace.yaml (1)
6-6: Verify the fixture path before updating the exclusionIt appears that
packages/nuxt/test/package-fixturedoes not exist in the checked-out repository, so the exclusion may be unnecessary or the path could be incorrect.• Confirm whether the
package-fixturedirectory (and any subfolders) should be present underpackages/nuxt/test/.
• If it is indeed intended to exist, update the exclusion to recursively omit all children:- - '!packages/nuxt/test/package-fixture' + - '!packages/nuxt/test/package-fixture/**'• If the fixtures live elsewhere or the path has changed, adjust the pattern accordingly or remove the redundant entry.
packages/nuxt/src/components/plugins/transform.ts (1)
1-2: Verify new runtime deps and cross-bundler compatibility
- Adding
exsolve(forresolveModulePath) and@vue/shared(isObject) in this hot path is fine, but please verify:
exsolveis already a dependency of the Nuxt package, or add it explicitly.- It works in both Vite and Webpack contexts during dev/build (no ESM/CJS interop surprises).
Would you like me to open a small follow-up to pin
exsolvein the package manifest and add a minimal smoke test for both bundlers?Also applies to: 9-10
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nuxt/test/components-transform.test.ts (2)
9-11: Give the virtual path an extension to better emulate real files.Some transforms in tooling rely on file extensions for heuristics. Using an extension (e.g.
.ts) reduces the risk of future false negatives if the transform tightens its guards.Apply this diff:
-const virtualFilePath = join(pkgPath, 'foo', 'bar', 'baz') +const virtualFilePath = join(pkgPath, 'foo', 'bar', 'baz.ts')
28-36: Prefer a direct undefined assertion over an inline snapshotThe inline snapshot of
undefinedcan be replaced with a clearer assertion:- expect(code).toMatchInlineSnapshot(`undefined`) + expect(code).toBeUndefined()No external fixture verification is required—this change is purely stylistic and reduces snapshot churn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/nuxt/test/components-transform.test.ts(2 hunks)
🧰 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/test/components-transform.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/nuxt/test/components-transform.test.ts
🧠 Learnings (2)
📓 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.
📚 Learning: 2025-07-18T16:46:07.446Z
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`
Applied to files:
packages/nuxt/test/components-transform.test.ts
🪛 GitHub Check: build
packages/nuxt/test/components-transform.test.ts
[failure] 9-9:
Cannot find name 'fileURLToPath'.
🪛 GitHub Check: code
packages/nuxt/test/components-transform.test.ts
[failure] 9-9: packages/nuxt/test/components-transform.test.ts
ReferenceError: fileURLToPath is not defined
❯ packages/nuxt/test/components-transform.test.ts:9:17
🪛 GitHub Actions: autofix.ci
packages/nuxt/test/components-transform.test.ts
[error] 9-9: ReferenceError: fileURLToPath is not defined. Ensure to import { fileURLToPath } from 'url' in this test setup. Command 'vitest run --project unit -u' failed.
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33049 will not alter performanceComparing Summary
Footnotes |
|
it feels like we shouldn't reimplement this logic. can we use exsolve to handle it? |
…ponents import mapping
ded605d to
b5905de
Compare
Ah, yes; |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/nuxt/test/package-fixture/package.jsonis excluded by!**/package.json
📒 Files selected for processing (3)
packages/nuxt/src/components/plugins/transform.ts(2 hunks)packages/nuxt/test/components-transform.test.ts(2 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pnpm-workspace.yaml
- packages/nuxt/test/components-transform.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/components/plugins/transform.ts
🧠 Learnings (3)
📓 Common learnings
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: 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.
📚 Learning: 2024-11-05T15:22:54.759Z
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.
Applied to files:
packages/nuxt/src/components/plugins/transform.ts
📚 Learning: 2024-12-12T12:36:34.871Z
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.
Applied to files:
packages/nuxt/src/components/plugins/transform.ts
🔇 Additional comments (1)
packages/nuxt/src/components/plugins/transform.ts (1)
1-1: LGTM: importingisObjectis appropriate for the runtime guardNo concerns here; usage below is correct.
🔗 Linked issue
Resolves #33032.
📚 Description
This PR adds a check to the components transform plugin's transform hook to detect whether a file is part of a package with a
package.jsonthat defines a#componentsimport mapping, in which case it is assumed the file's#componentsis referencing its package's internal import map.Currently, a file is not transformed only if it does not contain
#components. With this PR, in addition, when#componentsis found:package.jsonfile is looked uppackage.jsondefines a#componentsimport mapping, the file is not further processed to prevent its#componentsimport from being replaced with nuxt's registered components.If
package.jsonis not found, or it does not define a#componentsimport mapping, the file is normally processed.There's also a new test and a simple fixture with only a
package.jsonthat defines the#componentsimport mapping. The test ensures that code including#componentsin a file that is part of the package defined by the fixture'spackage.jsonwon't get transformed by the components transform plugin.