Skip to content
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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 13, 2023
Merged

fix(esbuild): enable experimentalDecorators by default #13805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
846bd09
fix(esbuild): enable experimentalDecorators by default
bluwy Jul 12, 2023
cbb24c4
fix: enable experimentalDecorators during scan
bluwy Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions packages/vite/src/node/optimizer/scan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,11 @@ async function prepareEsbuildScanner(

const plugin = esbuildScanPlugin(config, container, deps, missing, entries)

const { plugins = [], ...esbuildOptions } =
config.optimizeDeps?.esbuildOptions ?? {}
const {
plugins = [],
tsconfigRaw,
...esbuildOptions
} = config.optimizeDeps?.esbuildOptions ?? {}

return await esbuild.context({
absWorkingDir: process.cwd(),
Expand All @@ -219,6 +222,16 @@ async function prepareEsbuildScanner(
format: 'esm',
logLevel: 'silent',
plugins: [...plugins, plugin],
tsconfigRaw:
typeof tsconfigRaw === 'string'
? tsconfigRaw
: {
...tsconfigRaw,
compilerOptions: {
experimentalDecorators: true,
...tsconfigRaw?.compilerOptions,
},
},
Comment on lines +225 to +234
Copy link
Member

@patak-dev patak-dev Jul 13, 2023 •
edited
Loading

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):

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

Copy link
Member Author

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.

Copy link
Member

@sapphi-red sapphi-red Jul 13, 2023 •
edited
Loading

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

...esbuildOptions,
})
}
Expand Down
8 changes: 8 additions & 0 deletions packages/vite/src/node/plugins/esbuild.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,14 @@ export async function transformWithEsbuild(
compilerOptions.useDefineForClassFields = false
}

// esbuild v0.18 only transforms decorators when `experimentalDecorators` is set to `true`.
// To preserve compat with the esbuild breaking change, we set `experimentalDecorators` to
// `true` by default if it's unset.
// TODO: Remove this in Vite 5
if (compilerOptions.experimentalDecorators === undefined) {
compilerOptions.experimentalDecorators = true
}

// esbuild uses tsconfig fields when both the normal options and tsconfig was set
// but we want to prioritize the normal options
if (options) {
Expand Down
12 changes: 12 additions & 0 deletions playground/tsconfig-json/__tests__/tsconfig-json.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,16 @@ describe('transformWithEsbuild', () => {
'import { MainTypeOnlyClass } from "./not-used-type";',
)
})

test('experimentalDecorators', async () => {
const main = path.resolve(__dirname, '../src/decorator.ts')
const mainContent = fs.readFileSync(main, 'utf-8')
// Should not error when transpiling decorators
// TODO: In Vite 5, this should require setting `tsconfigRaw.experimentalDecorators`
// or via the closest `tsconfig.json`
const result = await transformWithEsbuild(mainContent, main, {
target: 'es2020',
})
expect(result.code).toContain('__decorateClass')
})
})
11 changes: 11 additions & 0 deletions playground/tsconfig-json/src/decorator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
function first() {
return function (...args: any[]) {}
}

export class Foo {
@first()
// @ts-expect-error we intentionally not enable `experimentalDecorators` to test esbuild compat
method(@first test: string) {
return test
}
}
1 change: 1 addition & 0 deletions playground/tsconfig-json/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// @ts-nocheck
import '../nested/main'
import '../nested-with-extends/main'
import './decorator'

// eslint-disable-next-line @typescript-eslint/consistent-type-imports
import { MainTypeOnlyClass } from './not-used-type'
Expand Down