-
Notifications
You must be signed in to change notification settings - Fork 370
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
feat: enable stage3 decorator for swc based on typescript version #5849
Conversation
|
decoratorVersion: | ||
!emitDecoratorMetadata && tsVersion >= 5 | ||
? '2022-03' | ||
: undefined, |
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.
This could be a breaking change for projects using ts^5 and emitDecoratorMetadata
not enabled?
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.
AFAIK this option takes effect only when decorator grammar is used?
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.
- Just took a look into the codebase, you're right that this PR won't break user when
emitDecoratorMetadata
is set. - However, as per https://github.com/web-infra-dev/modern.js/blob/main/packages/solutions/module-tools/src/builder/feature/index.ts#L35-L39, SWC transform is enabled only in certain conditions. New trigger condition is needed to enable the
'2022-03'
config. tsVersion > 5
is not sufficient, user could still use legacy decorator withexperimentalDecorators
flag, https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#differences-with-experimental-legacy-decorators.- esbuild start to support the stage3 decorator since https://github.com/evanw/esbuild/releases/v0.21.0 with heuristic legacy backward support. IMO, bumping esbuild to latest version could be a way more straightforward solution.
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.
🤔 I'm fine with either. If you are willing to add another trigger option I'd like to help implement it. If you can bump to latest esbuild that'd be great 😀
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.
I'd prefer bumping esbuild as we should catch up the latest version and the feature is resolved by the way. We're now using 0.19.2
version of esbuild which is release 10 months ago. It's better to bump it, @chenjiahan is there any issue blocks bumping esbuild that I don't know?
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.
Talked with @chenjiahan, there's no blocking point stopping us from bumping esbuild. We'd inclined to bump esbuild to address this issue.
I'm closing this PR in favor of bumping esbuild. Let me know if you're interested in implement this. Thanks for the PR anyway.
What's your opinion on the configuration option? |
Summary
This is a draft PR as the API is not designed. I'd like to discuss with the maintenance team if this implementation is fine.
Currently, I can hack it by enabling
transformLodash
to enable swc compiler.Related Links
Fixes #5834
Checklist
pnpm run change
.