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

Jump to bottom

fix(scanner): respect experimentalDecorators: true #15206

Merged
merged 6 commits into from Feb 21, 2024
Merged

fix(scanner): respect experimentalDecorators: true #15206

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5db7c1a
fix(scanner): respect `experimentalDecorators: true`
sapphi-red Dec 1, 2023
cd641b0
chore: fix typecheck
sapphi-red Dec 2, 2023
f1d40a9
fix: add resolveDir to `namespace: 'html'`
sapphi-red Dec 2, 2023
b1cac17
chore: merge main
sapphi-red Feb 20, 2024
2077ede
chore: revert changes to esbuild plugin
sapphi-red Feb 20, 2024
10f7e4c
fix: load root tsconfig experimentalDecorators option
sapphi-red Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions packages/vite/src/node/optimizer/scan.ts
View file
Open in desktop
Expand Up @@ -485,6 +485,7 @@ function esbuildScanPlugin(
return {
loader: 'js',
contents: js,
resolveDir: normalizePath(path.dirname(p)),
}
}

Expand Down Expand Up @@ -606,12 +607,15 @@ function esbuildScanPlugin(
return externalUnlessEntry({ path: id })
}

const namespace = htmlTypesRE.test(resolved) ? 'html' : undefined

return {
path: path.resolve(cleanUrl(resolved)),
namespace,
if (htmlTypesRE.test(resolved)) {
return {
path: path.resolve(cleanUrl(resolved)),
namespace: 'html',
}
}

// make esbuild handle js/ts/jsx/tsx
// so that tsconfig.json is respected by esbuild
Copy link
Member

@bluwy bluwy Dec 4, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this mean that we let esbuild resolve instead? Wouldn't that possibly diverge from Vite's resolver that we want to use instead?

Maybe we can fix this issue by enabling experimentalDecorators by default only for scanning Looks like I explicitly removed experimentalDecorators support in scanning. Hmm I'm confused though why esbuild doesn't respect the tsconfig if we manually resolve.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm true.
It seems esbuild doesn't support reading tsconfig.json if the plugin resolved the path (evanw/esbuild#2265).

related code: the part resolving tsconfig, resolver (res.Resolve calls finalizeResolve which resolves tsconfig)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I'd like to avoid it, maybe reverting and enabling back experimentalDecorators for scanning would be the easier way for now 🤔 I think it shouldn't hurt too much since we only need to scan things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading microsoft/TypeScript#50820, it seems there's some incompatibilities between the syntax of them unfortunately. 😢
Example 1 .(private fields), Example 2 (class expressions) (the error disappears if experimentalDecorators is set to false)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, perhaps what we can do for now is manually read the project's tsconfig.json and then infer experimentalDecorators from there? Seems like there will always be a tradeoff, but worst case they could manually disable/enable it with esbuildOptions.tsconfigRaw perhaps.

} else {
// resolve failed... probably unsupported type
return externalUnlessEntry({ path: id })
Expand Down
12 changes: 11 additions & 1 deletion playground/tsconfig-json/__tests__/tsconfig-json.spec.ts
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ import path from 'node:path'
import fs from 'node:fs'
import { transformWithEsbuild } from 'vite'
import { describe, expect, test } from 'vitest'
import { browserLogs } from '~utils'
import { browserLogs, isServe, serverLogs } from '~utils'

test('should respected each `tsconfig.json`s compilerOptions', () => {
// main side effect should be called (because of `"importsNotUsedAsValues": "preserve"`)
Expand All @@ -21,6 +21,16 @@ test('should respected each `tsconfig.json`s compilerOptions', () => {
expect(browserLogs).toContain('data setter in NestedWithExtendsBase')
})

test.runIf(isServe)('scanner should not error with decorators', () => {
expect(serverLogs).not.toStrictEqual(
expect.arrayContaining([
expect.stringContaining(
'Parameter decorators only work when experimental decorators are enabled',
),
]),
)
})

describe('transformWithEsbuild', () => {
test('merge tsconfigRaw object', async () => {
const main = path.resolve(__dirname, '../src/main.ts')
Expand Down
4 changes: 2 additions & 2 deletions playground/tsconfig-json/src/decorator.ts
View file
Open in desktop
@@ -1,11 +1,11 @@
// @ts-nocheck playground/tsconfig.json does not have decorators enabled
function first() {
return function (...args: any[]) {}
}

export class Foo {
@first()
// @ts-expect-error we intentionally not enable `experimentalDecorators` to test esbuild compat
bluwy marked this conversation as resolved.
Show resolved Hide resolved
method(@first test: string) {
method(@first() test: string) {
return test
}
}