-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
fix(esbuild): enable experimentalDecorators by default #13805
Conversation
Run & review this pull request in StackBlitz Codeflow. |
Pushed a commit to enable I didn't go with that fix for now as it might affect scanning perf, or cause another unexpected behaviour. We should be able to circle back on this in Vite 5 as an error will appear when we remove the |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
tsconfigRaw: | ||
typeof tsconfigRaw === 'string' | ||
? tsconfigRaw | ||
: { | ||
...tsconfigRaw, | ||
compilerOptions: { | ||
experimentalDecorators: true, | ||
...tsconfigRaw?.compilerOptions, | ||
}, | ||
}, |
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 wonder if we should use (and only add the prop if tsconfig
isn't defined):
tsconfigRaw: | |
typeof tsconfigRaw === 'string' | |
? tsconfigRaw | |
: { | |
...tsconfigRaw, | |
compilerOptions: { | |
experimentalDecorators: true, | |
...tsconfigRaw?.compilerOptions, | |
}, | |
}, | |
tsconfigRaw: | |
tsconfigRaw ?? { | |
compilerOptions: { | |
experimentalDecorators: true, | |
}, | |
}, | |
}, |
so it works consistently for the three cases, experimentalDecorators
is only set if the user doesn't manually modify the config.
tsconfigRaw
is an objecttsconfigRaw
is a stringtsconfig
passed as a path
Right now, only the object form is respected. Or the other way around, the file or string could be first converted to an object.
I'm fine moving forward with the current patch though if we consider it as a temporal solution to help the majority of users. We could add a comment here in that case
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.
Wouldn't that not apply the experimentalDecorators: true
default if the user pass something like { compilerOptions: { useDefineForClassFields: true } }
. Currently in transformWithEsbuild
having that config would fallback with experimentalDecorators: true
too. I think for string form it's fine to respect the user input completely like transformWithEsbuild
does for now.
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 guess it would be a bit complicated to do it correct, since we will have to load/parse the tsconfig.
I think it's ok with this implementation if we add a small section to the changelog.
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.
Oh you're right that tsconfigRaw
would've overwrite tsconfig
if only tsconfig
is set (iirc) 🤔 I think we can not set tsconfigRaw
if tsconfig
is specified?
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.
Yes, it won't apply it in that case. I find it inconsistent to only do it for the object form, so I was trying to see what would be correct here. But I was thinking along the lines of what @sapphi-red said. Let's merge this one as is for now then.
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.
Oh, it seems an error happens. esbuild repl
So we need to skip adding tsconfigRaw
if tsconfig
exists.
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.
Ah ok I can fix that in a followup PR now.
tsconfigRaw: | ||
typeof tsconfigRaw === 'string' | ||
? tsconfigRaw | ||
: { | ||
...tsconfigRaw, | ||
compilerOptions: { | ||
experimentalDecorators: true, | ||
...tsconfigRaw?.compilerOptions, | ||
}, | ||
}, |
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 guess it would be a bit complicated to do it correct, since we will have to load/parse the tsconfig.
I think it's ok with this implementation if we add a small section to the changelog.
If one wants to use the non-experimental decorators from TypeScript 5 (which are different), how would that work now that the experimental ones are enabled by default? |
IIUC you can still use the non-experimental decorators, just that experimental decorators extends some non-standard syntax. That was also why I didn't caught the second bug in the issue as I was only testing the standard decorators. Note though that esbuild only implements pass-through mode for them in |
It still doesn't seem to work. The property decorator is invalid |
@spencer17x would you create a new issue with a minimal reproduction against [email protected]? |
Description
(potential) fix #13736. however there were some reports that this doesn't work, so probably worth holding off this PR for now.
follow up on #13525, not setting
experimentalDecorators
by default seems to be causing issues at the moment, it might be better to turn this on by default for now and disable it in Vite 5 again.Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).