feat(kit,nuxt,schema): support experimental decorators syntax#27672
feat(kit,nuxt,schema): support experimental decorators syntax#27672
Conversation
|
|
|
Is enabling this setting still necessary when using the stage 3 decorator proposal / decorators since TS 5.0? https://chatgpt.com/share/e21d39be-259d-4dc2-b426-295b78240e70 |
pi0
left a comment
There was a problem hiding this comment.
(adding nitro.experimental.decorators could be also nice for future compat)
|
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
|
Would love seeing this in a new release |
|
waiting for #30066 |
|
Warning Rate limit exceeded@danielroe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes implement support for experimental decorators across multiple packages and configurations. A new boolean flag is introduced in the template to control TypeScript compiler options related to decorators. Several modules have updated their import and usage of the 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
test/fixtures/basic/server/api/experimental/decorators.ts (1)
2-4: Consider adding type safety to the decorator function.The decorator function could benefit from stronger typing to ensure type safety at compile time.
- function something (_method: () => unknown) { + function something<T extends () => unknown>(_method: T): T { - return () => 'decorated' + return (() => 'decorated') as T }test/fixtures/basic/pages/experimental/decorators.vue (2)
15-15: Consider handling fetch errors.The
useFetchcall should include error handling for robustness.-const { data } = await useFetch('/api/experimental/decorators') +const { data, error } = await useFetch('/api/experimental/decorators')Then update the template to handle errors:
<template> <div> - {{ value }}-{{ data }} + <div v-if="error">Error: {{ error.message }}</div> + <div v-else>{{ value }}-{{ data }}</div> </div> </template>
24-26: Consider removing empty style block.The empty scoped style block can be removed if not needed.
packages/schema/src/config/esbuild.ts (2)
15-27: Consider caching the decorator configuration check.The
useDecoratorscheck might be called frequently during builds. Consider caching the result.+let cachedDecoratorConfig: boolean | undefined + tsconfigRaw: { $resolve: async (val: TransformOptions['tsconfigRaw'], get) => { - const useDecorators = await get('experimental').then(r => (r as Record<string, unknown>)?.decorators === true) + if (cachedDecoratorConfig === undefined) { + cachedDecoratorConfig = await get('experimental').then(r => (r as Record<string, unknown>)?.decorators === true) + } + const useDecorators = cachedDecoratorConfig if (!useDecorators) { return val }
20-26: Consider adding JSDoc for compiler options.The compiler options would benefit from documentation explaining their purpose.
return defu(val, { compilerOptions: { + // Required for proper decorator metadata handling useDefineForClassFields: false, + // Disable legacy experimental decorators as we're using the stage 3 proposal experimentalDecorators: false, }, } satisfies TransformOptions['tsconfigRaw'])packages/nuxt/src/core/plugins/prehydrate.ts (1)
5-5: LGTM! Improved code organisation by using local utility functions.The change consolidates transformation logic into local utilities, making the code more maintainable.
Consider adding JSDoc comments to document the imported utility functions for better code documentation:
+/** Utilities for parsing and transforming code */ import { parseAndWalk, transform, withLocations } from '../../core/utils/parse'packages/nuxt/src/pages/route-rules.ts (1)
7-7: LGTM! Consistent use of local utility functions.The change maintains consistency by using the same local utilities for code transformation.
Consider adding type annotations for better code maintainability:
-import { parseAndWalk, transform } from '../core/utils/parse' +import type { TransformResult } from '../core/utils/parse' +import { parseAndWalk, transform } from '../core/utils/parse'packages/schema/src/config/experimental.ts (1)
94-99: LGTM! Consider enhancing the documentation.The addition of the
decoratorsproperty is well-documented. Consider adding more details about:
- The stage of the TC39 proposal
- Any limitations or known issues
- Migration guidance for users
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/schema/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (16)
packages/kit/src/template.ts(2 hunks)packages/nuxt/src/core/plugins/plugin-metadata.ts(1 hunks)packages/nuxt/src/core/plugins/prehydrate.ts(1 hunks)packages/nuxt/src/core/utils/parse.ts(1 hunks)packages/nuxt/src/pages/route-rules.ts(1 hunks)packages/nuxt/src/pages/utils.ts(1 hunks)packages/schema/build.config.ts(1 hunks)packages/schema/src/config/esbuild.ts(1 hunks)packages/schema/src/config/experimental.ts(1 hunks)packages/schema/src/config/index.ts(2 hunks)packages/schema/src/config/vite.ts(3 hunks)packages/schema/src/config/webpack.ts(1 hunks)test/basic.test.ts(1 hunks)test/fixtures/basic/nuxt.config.ts(1 hunks)test/fixtures/basic/pages/experimental/decorators.vue(1 hunks)test/fixtures/basic/server/api/experimental/decorators.ts(1 hunks)
🔇 Additional comments (15)
test/fixtures/basic/server/api/experimental/decorators.ts (1)
7-10: LGTM! Decorator implementation follows the stage 3 proposal.The decorator syntax and usage align with the stage 3 decorator proposal specifications.
test/fixtures/basic/pages/experimental/decorators.vue (1)
2-4: Consider adding type safety to the decorator function.Similar to the server-side implementation, the decorator function could benefit from stronger typing.
packages/schema/src/config/index.ts (1)
6-6: LGTM! Clean integration of esbuild configuration.The import and export of esbuild configuration maintain the existing pattern and alphabetical order.
Also applies to: 30-30
packages/schema/build.config.ts (1)
42-42: LGTM! Addition of esbuild as an external dependency.The addition of 'esbuild' as an external dependency aligns with the PR's objective of enabling experimental decorators syntax.
packages/schema/src/config/vite.ts (3)
2-2: LGTM! Added defu for options merging.The addition of defu is appropriate for merging configuration options.
98-100: LGTM! Added esbuild options resolution.The change allows for flexible configuration of esbuild options while maintaining defaults.
Let's verify the esbuild options schema:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if esbuild schema is properly defined fd --type f --extension ts "esbuild.ts" --exec cat {} \;Length of output: 2246
Verification Successful: Esbuild Options Schema Defined Correctly
The schema file (esbuild.ts) defines the esbuild options correctly. The async
$resolvefunction fortsconfigRawproperly utilisesdefuto merge user-specified values with defaults when the experimental decorators are in use. This behaviour aligns with the intended design mentioned in the review comment.
- The
$resolvefunction intsconfigRawcorrectly conditionally merges configuration based on theexperimentalsetting.- Default esbuild options (such as
jsxFactory,jsxFragment) are accurately specified.
86-88: LGTM! Added esbuildOptions resolution.The use of defu for merging esbuild options with defaults is a good practice.
Let's verify that the esbuild options are properly typed:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if esbuild options type is properly defined ast-grep --pattern 'interface ViteConfig { $$$ optimizeDeps?: { $$$ esbuildOptions?: { $$$ } $$$ } $$$ }'Length of output: 137
Manual Verification Required: Esbuild Options Typing
The ast-grep search did not return any results indicating an explicit type definition for
esbuildOptionswithin the expected location (i.e. inside theoptimizeDepsproperty ofViteConfig). This could mean that the type is defined elsewhere or that its typing is implicit. Please manually verify that theesbuildOptionsproperty is correctly and intentionally typed.packages/nuxt/src/core/plugins/plugin-metadata.ts (1)
10-10: LGTM! Import refactoring looks good.The change to import
transformfrom the local parse utility module aligns with the decorator support implementation.test/fixtures/basic/nuxt.config.ts (1)
162-162: LGTM! Experimental feature flag added correctly.The addition of the decorators flag in the experimental section is appropriate, as decorators are currently a stage 3 proposal.
packages/schema/src/config/webpack.ts (1)
161-163: LGTM! Dynamic esbuild configuration looks good.The change to use dynamic resolution for esbuild loader options allows for proper integration of decorator support.
packages/kit/src/template.ts (2)
183-183: LGTM! Feature flag check implemented correctly.The boolean flag to check for experimental decorators is well-placed and properly derived from the Nuxt options.
203-216: LGTM! TypeScript configuration for decorators is comprehensive.The configuration correctly sets up TypeScript for the stage 3 decorators proposal:
- Disables useDefineForClassFields and experimentalDecorators (as we're using the new syntax)
- Adds esnext.decorators to the lib array for proper type definitions
packages/nuxt/src/core/utils/parse.ts (1)
3-3: LGTM! Imports are well-organized.The imports are properly grouped and named.
Also applies to: 6-7
packages/nuxt/src/pages/utils.ts (1)
14-14: LGTM! Import addition is correct.The
transformfunction is properly imported alongside related functions.test/basic.test.ts (1)
2784-2790: LGTM! Well-structured test cases for experimental decorators support.The test cases effectively verify the decorator functionality by checking both server-side rendering and client-side hydration.
| export async function transform<T extends TransformOptions> (input: string | Uint8Array, options?: SameShape<TransformOptions, T>): Promise<TransformResult<T>> { | ||
| return await esbuildTransform(input, { ...tryUseNuxt()?.options.esbuild.options, ...options }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the case where tryUseNuxt() returns undefined.
The current implementation might cause issues if tryUseNuxt() returns undefined. Consider providing a fallback.
Apply this diff to handle the edge case:
- return await esbuildTransform(input, { ...tryUseNuxt()?.options.esbuild.options, ...options })
+ const nuxtOptions = tryUseNuxt()?.options.esbuild.options ?? {}
+ return await esbuildTransform(input, { ...nuxtOptions, ...options })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function transform<T extends TransformOptions> (input: string | Uint8Array, options?: SameShape<TransformOptions, T>): Promise<TransformResult<T>> { | |
| return await esbuildTransform(input, { ...tryUseNuxt()?.options.esbuild.options, ...options }) | |
| } | |
| export async function transform<T extends TransformOptions> (input: string | Uint8Array, options?: SameShape<TransformOptions, T>): Promise<TransformResult<T>> { | |
| const nuxtOptions = tryUseNuxt()?.options.esbuild.options ?? {} | |
| return await esbuildTransform(input, { ...nuxtOptions, ...options }) | |
| } |
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #27672 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/2.guide/3.going-further/1.experimental-features.md (2)
478-478: Clarify Redundant Verb UsageThe sentence currently reads "This option enables enabling decorator syntax…". Please remove the duplicate "enables" (e.g. change to "This option enables decorator syntax…") to improve clarity.
🧰 Tools
🪛 LanguageTool
[grammar] ~478-~478: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...-only page. ## decorators This option enables enabling decorator syntax across your entire Nux...(REPEATED_VERBS)
484-486: Refine Warning Block PunctuationThere are minor punctuation issues in the warning block. Consider reviewing the punctuation (especially around the note in line 485) for improved readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~485-~485: Loose punctuation mark.
Context: ... this finally lands in the JS standard. :: #### Usage ```ts twoslash [nuxt.conf...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/2.guide/3.going-further/1.experimental-features.md(1 hunks)packages/kit/src/resolve.test.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/2.guide/3.going-further/1.experimental-features.md
[grammar] ~478-~478: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...-only page. ## decorators This option enables enabling decorator syntax across your entire Nux...
(REPEATED_VERBS)
[uncategorized] ~483-~483: Loose punctuation mark.
Context: ...xperimentalDecorators` implementation. ::warning Note that there may be changes ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~485-~485: Loose punctuation mark.
Context: ... this finally lands in the JS standard. :: #### Usage ```ts twoslash [nuxt.conf...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
docs/2.guide/3.going-further/1.experimental-features.md
488-488: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🪛 GitHub Actions: autofix.ci
docs/2.guide/3.going-further/1.experimental-features.md
[error] 488-488: Heading levels should only increment by one level at a time [Expected: h3; Actual: h4]
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: codeql (actions)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: code
🔇 Additional comments (2)
docs/2.guide/3.going-further/1.experimental-features.md (2)
476-477: New "decorators" Section AddedThe introduction of the "decorators" section is well placed and clearly labelled. It fits with the other experimental features in the document, enhancing clarity for users exploring new options.
490-513: Usage Examples Are Clear and InformativeThe provided code examples for enabling decorators in the Nuxt configuration and demonstrating their application in a class (in app.vue) are clear and effectively illustrate the new feature. This will help users understand how to implement the experimental decorators.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/schema/src/config/esbuild.ts (1)
5-32: Consider adding JSDoc comments for configuration options.While the code is well-structured, adding JSDoc comments for each configuration option would improve maintainability.
Apply this diff to add documentation:
export default defineResolvers({ esbuild: { + /** + * Shared esbuild options used across Nuxt and passed to other builders. + * @property {string} jsxFactory - The factory function used for JSX elements + * @property {string} jsxFragment - The component used for JSX fragments + * @property {object} tsconfigRaw - TypeScript compiler options + */ options: { jsxFactory: 'h', jsxFragment: 'Fragment',docs/2.guide/3.going-further/1.experimental-features.md (1)
478-478: Fix grammatical redundancy.The phrase "enables enabling" is redundant.
Apply this diff to fix the grammar:
-This option enables enabling decorator syntax across your entire Nuxt/Nitro app, powered by [esbuild](https://github.com/evanw/esbuild/releases/tag/v0.21.3). +This option enables decorator syntax across your entire Nuxt/Nitro app, powered by [esbuild](https://github.com/evanw/esbuild/releases/tag/v0.21.3).🧰 Tools
🪛 LanguageTool
[grammar] ~478-~478: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...-only page. ## decorators This option enables enabling decorator syntax across your entire Nux...(REPEATED_VERBS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/schema/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (6)
docs/2.guide/3.going-further/1.experimental-features.md(1 hunks)packages/schema/src/config/esbuild.ts(1 hunks)packages/schema/src/config/experimental.ts(1 hunks)packages/schema/src/config/index.ts(2 hunks)packages/schema/src/config/vite.ts(3 hunks)packages/schema/src/config/webpack.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/schema/src/config/experimental.ts
- packages/schema/src/config/index.ts
- packages/schema/src/config/webpack.ts
🧰 Additional context used
🪛 LanguageTool
docs/2.guide/3.going-further/1.experimental-features.md
[grammar] ~478-~478: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...-only page. ## decorators This option enables enabling decorator syntax across your entire Nux...
(REPEATED_VERBS)
[uncategorized] ~483-~483: Loose punctuation mark.
Context: ...xperimentalDecorators` implementation. ::warning Note that there may be changes ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~485-~485: Loose punctuation mark.
Context: ... this finally lands in the JS standard. :: ### Usage ```ts twoslash [nuxt.confi...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (4)
packages/schema/src/config/esbuild.ts (1)
1-4: LGTM!The imports and type usage are correct and well-structured.
packages/schema/src/config/vite.ts (2)
90-92: LGTM!The
esbuildOptionsproperty correctly merges with global esbuild options usingdefu.
102-104: LGTM!The esbuild resolver function correctly merges configuration values with global esbuild options.
docs/2.guide/3.going-further/1.experimental-features.md (1)
488-488: Adjust heading level for consistency.The "Usage" heading increases the heading level by two steps from the preceding "decorators" section.
|
So that at the moment is not usefully? It not work with decorators from other packages like mikro-orm? |
|
@Maakdev not sure what you mean, but feel free to open an issue with a minimal reproduction if this isn't working for you |
|
@danielroe i do #31315 i mean Decorators not work, warnings, not compatible, not work without emitDecoratorMetadata as prior |
|
@danielroe also fetch not work in prod mode, in dev if i click button http request is made, in prod no errors, no request, nothing. Button not work. It's because of use of inversify to inject pinia stores and decorators. But it work in dev mode (with emitDecoratorMetadata not experimental decorators ). And not in prod :( So big difference between dev and prod and pinia store is a class |
|
if i put in template only in dev all work @pi0 @danielroe what is with that framework? |
|
please comment on an issue rather than a merged pr, and also please avoid directly tagging maintainers 🙏 |
🔗 Linked issue
resolves #14126
📚 Description
This PR enables enabling decorator syntax across your entire Nuxt/Nitro app, powered by esbuild. It also allows passing and customising esbuild options across nitro, webpack, and vite build/optimizeDeps configurations via a new top-level
esbuildoption in yournuxt.configfile.Usage
Background
For a long time, TypeScript has had support for decorators via
compilerOptions.experimentalDecorators. This implementation predated the TC39 standardization process. Now, decorators are a Stage 3 Proposal, and supported without special configuration in TS 5.0+ (see microsoft/TypeScript#52582 and https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#decorators).Caution
Note that there may be changes before this finally lands in the JS standard.
Caution
If you have previously used
experimentalDecoratorsfrom TypeScript, the implementation and types are different. Please read the proposal before enabling.TODO:
esbuild