fix(esbuild): enable experimentalDecorators by default by bluwy · Pull Request #13805 · vitejs/vite

bluwy · 2023-07-12T08:44:16Z

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?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the PR Title Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

bolt-new-by-stackblitz · 2023-07-12T08:44:18Z

Run & review this pull request in StackBlitz Codeflow.

bluwy · 2023-07-13T06:13:03Z

Pushed a commit to enable experimentalDecorators during scan too. It turns out the scanner doesn't really use the project's tsconfig.json at the moment because we're passing stdin. If I pass an explicit tsconfig: 'tsconfig.json' config to esbuild, only then it detects it.

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 experimentalDecorators default.

patak-dev · 2023-07-13T09:28:39Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-07-13T09:29:08Z

📝 Ran ecosystem CI: Open

suite	result
analogjs	✅ success
astro	✅ success
histoire	❌ failure
iles	❌ failure
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt	✅ success
nx	✅ success
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	✅ success
unocss	✅ success
vite-plugin-pwa	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success

patak-dev · 2023-07-13T10:00:58Z

packages/vite/src/node/optimizer/scan.ts

+    tsconfigRaw:
+      typeof tsconfigRaw === 'string'
+        ? tsconfigRaw
+        : {
+            ...tsconfigRaw,
+            compilerOptions: {
+              experimentalDecorators: true,
+              ...tsconfigRaw?.compilerOptions,
+            },
+          },


I wonder if we should use (and only add the prop if tsconfig isn't defined):

Suggested change

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 object

tsconfigRaw is a string

tsconfig 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

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.

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.

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?

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.

Oh, it seems an error happens. esbuild repl
So we need to skip adding tsconfigRaw if tsconfig exists.

Ah ok I can fix that in a followup PR now.

sapphi-red · 2023-07-13T12:46:04Z

packages/vite/src/node/optimizer/scan.ts

+    tsconfigRaw:
+      typeof tsconfigRaw === 'string'
+        ? tsconfigRaw
+        : {
+            ...tsconfigRaw,
+            compilerOptions: {
+              experimentalDecorators: true,
+              ...tsconfigRaw?.compilerOptions,
+            },
+          },


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.

hybridherbst · 2023-07-13T13:36:55Z

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?

bluwy · 2023-07-14T06:37:42Z

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 esbuild.target: 'esnext'. It doesn't down-level the syntax yet. A future release would improve this.

spencer17x · 2023-07-17T09:09:37Z

It still doesn't seem to work. The property decorator is invalid

patak-dev · 2023-07-17T09:10:56Z

@spencer17x would you create a new issue with a minimal reproduction against [email protected]?

spencer17x · 2023-07-17T09:23:27Z

#13874 @spencer17x

fix(esbuild): enable experimentalDecorators by default

846bd09

patak-dev previously approved these changes Jul 12, 2023

View reviewed changes

fix: enable experimentalDecorators during scan

cbb24c4

bluwy dismissed patak-dev’s stale review via cbb24c4 July 13, 2023 06:08

patak-dev added p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release labels Jul 13, 2023

patak-dev reviewed Jul 13, 2023

View reviewed changes

sapphi-red approved these changes Jul 13, 2023

View reviewed changes

patak-dev merged commit e8880f0 into main Jul 13, 2023

patak-dev deleted the enable-experimental-decorators branch July 13, 2023 12:52

bluwy mentioned this pull request Jul 13, 2023

fix(scan): skip tsconfigRaw fallback if tsconfig is set #13823

Merged

4 tasks

sapphi-red mentioned this pull request Jul 29, 2023

fix(optimizer): enable experimentalDecorators by default #13981

Merged

9 tasks

bluwy mentioned this pull request Oct 30, 2023

refactor(esbuild)!: remove esbuild 0.17 -> 0.18 compat #14804

Merged

9 tasks

sapphi-red mentioned this pull request Dec 1, 2023

fix(scanner): respect experimentalDecorators: true #15206

Merged

9 tasks

Uh oh!

Comments

Conversation

bluwy commented Jul 12, 2023

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

bolt-new-by-stackblitz bot commented Jul 12, 2023

Uh oh!

bluwy commented Jul 13, 2023

Uh oh!

patak-dev commented Jul 13, 2023

Uh oh!

vite-ecosystem-ci bot commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patak-dev Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluwy Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

sapphi-red Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluwy Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

patak-dev Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

sapphi-red Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

bluwy Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

sapphi-red Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hybridherbst commented Jul 13, 2023

Uh oh!

bluwy commented Jul 14, 2023

Uh oh!

spencer17x commented Jul 17, 2023

Uh oh!

patak-dev commented Jul 17, 2023

Uh oh!

spencer17x commented Jul 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vite-ecosystem-ci bot commented Jul 13, 2023 •

edited

Loading

patak-dev Jul 13, 2023 •

edited

Loading

sapphi-red Jul 13, 2023 •

edited

Loading

sapphi-red Jul 13, 2023 •

edited

Loading