-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
fix(nuxt): allow modules to add to typescript.hoist
#32601
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
|
WalkthroughThe update restructures the TypeScript type path resolution process within the ✨ 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.
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)
packages/nuxt/src/core/nuxt.ts (1)
981-1009
: Well-structured TypeScript path resolution function.The extracted function properly handles:
- Nested package resolution using the regex pattern
- Exclusion of packages present in
package.json
dependencies- Nightly package aliasing with deduplication
- Efficient flattening of Promise results
The function signature and implementation align well with TypeScript conventions and the PR objectives.
Consider adding JSDoc comments for better documentation:
+/** + * Resolves TypeScript path mappings for hoisted packages + * @param nuxt - The Nuxt instance + * @returns Promise resolving to TypeScript path mappings + */ async function resolveTypescriptPaths (nuxt: Nuxt): Promise<Record<string, [string]>> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/core/nuxt.ts
(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
**/*.{ts,tsx,vue}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 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.
Learnt from: GalacticHypernova
PR: nuxt/nuxt#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: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:48:28.134Z
Learning: Applies to **/*.vue : Use `<script setup lang="ts">` and the composition API when creating Vue components
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:48:28.134Z
Learning: Applies to **/*.{ts,tsx} : Follow standard TypeScript conventions and best practices
Learnt from: TheAlexLichter
PR: nuxt/nuxt#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.
packages/nuxt/src/core/nuxt.ts (7)
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: GalacticHypernova
PR: nuxt/nuxt#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: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:48:28.134Z
Learning: Applies to **/*.{ts,tsx} : Follow standard TypeScript conventions and best practices
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:48:28.134Z
Learning: Applies to **/*.vue : Use `<script setup lang="ts">` and the composition API when creating Vue components
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:48:28.134Z
Learning: Applies to **/*.{ts,tsx,vue} : Remove code that is not used or needed
Learnt from: TheAlexLichter
PR: nuxt/nuxt#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.
⏰ 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). (4)
- GitHub Check: codeql (actions)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (3)
packages/nuxt/src/core/nuxt.ts (3)
222-228
: Excellent refactoring with proper lazy initialisation.The lazy initialisation pattern with
paths ||= await resolveTypescriptPaths(nuxt)
efficiently prevents redundant path resolution whilst ensuring the paths are available when needed in thenitro:config
hook.
231-231
: Correctly updated hook to async for path resolution.Making the
prepare:types
hook asynchronous is necessary to properly await the TypeScript path resolution.
253-253
: Consistent lazy initialisation pattern.The same lazy initialisation approach is used here as in the
nitro:config
hook, ensuring consistent behaviour and avoiding duplicate path resolution.
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #32601 will not alter performanceComparing Summary
|
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.
Great! Tested with storybook reproduction project.
Tests flaky or did something actually break? |
flaky tests for some reason |
### 🔗 Linked issue * #920 <!-- If it resolves an open issue, please link the issue here. For example "Resolves #123" --> ### 📚 Description With nuxt/nuxt#32601 merged, the related issue will be resolved for projects using nuxt 4. This will also improve/add support for the directory structure in nuxt 4 as the `.storybook` directory is outside the app/server directories and needs to be added manually by this module. ~~If we also want this resolved for projects using nuxt 3, the PR will either need to be backported (ideal) or we could use my package nuxt-module-utils (I have not done so in this PR, I don't want to be pushy with my own packages 😂).~~ (this is not relevant since the separated tsconfig structure is nuxt 4 only) <!-- Describe your changes in detail --> <!-- Why is this change required? What problem does it solve? --> --------- Co-authored-by: Tobias Diez <code@tobiasdiez.de>
🔗 Linked issue
📚 Description
nuxt-modules/storybook#920 (comment)
this allows modules to push to
typescript.hoist
for key dependencies which they expect users to be able to import from or augment.