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.

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!: update esbuild to 0.18.2 #13525

Merged
merged 7 commits into from Jun 19, 2023
Merged

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jun 15, 2023

Description

This PR updates esbuild to ^0.18.2 from ^0.17.5.
https://github.com/evanw/esbuild/blob/main/CHANGELOG.md

The breaking changes included is:

If a user is using dep optimization with TypeScript files, almost all the breaking changes are related.
https://github.com/evanw/esbuild/blob/main/CHANGELOG.md#0180

fixes #13500
fixes #13391

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.

@sapphi-red sapphi-red added breaking change p3-significant High priority enhancement (priority) labels Jun 15, 2023
@stackblitz
Copy link

stackblitz bot commented Jun 15, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red sapphi-red added this to the 4.4 milestone Jun 15, 2023
@sapphi-red

This comment was marked as duplicate.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as duplicate.

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jun 15, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ⏹️ cancelled
astro ❌ failure
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

Co-authored-by: patak <matias.capeletto@gmail.com>
@sapphi-red
Copy link
Member Author

sapphi-red commented Jun 15, 2023

analogjs is failing with ERROR: Experimental decorators are not currently enabled error.
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/5276950022/jobs/9544390272#step:8:802

astro is failing with ReferenceError: window is not defined.
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/5276950022/jobs/9544390386#step:8:2634

histoire and iles is failing with main branch, too.

@sapphi-red
Copy link
Member Author

@bluwy I don't understand why this PR fails with astro. Would you take a look?

@patak-dev
Copy link
Member

analogjs is failing with ERROR: Experimental decorators are not currently enabled error.
vitejs/vite-ecosystem-ci/actions/runs/5276950022/jobs/9544390272#step:8:802

I wonder if this would affect other Angular projects. Maybe we could add "experimentalDecorators": true on our side and do the breaking change in Vite 5? Is there a benefit to doing it in the 4.4?

@sapphi-red
Copy link
Member Author

sapphi-red commented Jun 15, 2023

I wonder if this would affect other Angular projects.

I guess there won't be many projects. Because if "experimentalDecorators": true is not set, TypeScript lower than v5 will show a warning, Typescript 5+ will show a type error.

Is there a benefit to doing it in the 4.4?

This breaking chage is required for esbuild's ES decorators support: evanw/esbuild#104 (comment)
The benefit is that we might get support for that in 4.4. Though, I don't know how much ES decorators is used.
One more benefit is the consistency with esbuild. It might be confusing that Vite only supports experimental decorators while esbuild supports ES decorators.

That said, I think it's also fine to delay this breaking change.

@patak-dev
Copy link
Member

I guess there won't be many projects. Because if "experimentalDecorators": true is not set, TypeScript lower than v5 will show a warning, Typescript 5+ will show a type error.

I think this is enough, let's align with esbuild as you proposed. cc @brandonroberts

@brandonroberts
Copy link

brandonroberts commented Jun 15, 2023

I guess there won't be many projects. Because if "experimentalDecorators": true is not set, TypeScript lower than v5 will show a warning, Typescript 5+ will show a type error.

I think this is enough, let's align with esbuild as you proposed. cc @brandonroberts

Thanks. There isn't a good reason experimentalDecorators: true wouldn't be set in an Angular project.

angular/angular#48096

@bluwy
Copy link
Member

bluwy commented Jun 19, 2023

@bluwy I don't understand why this PR fails with astro. Would you take a look?

@sapphi-red The problem seems to be that esbuild and this PR now picks up verbatimModuleSyntax, derived from this tsconfig during the tests. And this value affects this transformWithEsbuild usage.

Note that there's a comment that it relied on importsNotUsedAsValues: 'remove' for unused import elision, but now the tsconfig has verbatimModuleSyntax: true that overrides that, preventing the elision. You can check why the elision is needed in this playground, where we want to remove the client:only import side-effects.

In Astro if I add verbatimModuleSyntax: false to have a higher override, it fixes the issue, which we can do if we go forward with this PR.

@sapphi-red
Copy link
Member Author

Ah, I see. Thank you for the explanation!

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

We think we should try this update in 4.4, given that Vite 5 is still 3 months away. We can give a longer testing window, and release 4.4 in the first days of July

@patak-dev patak-dev merged commit ab967c0 into vitejs:main Jun 19, 2023
13 checks passed
@sapphi-red sapphi-red deleted the feat/esbuild-0.18 branch June 20, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p3-significant High priority enhancement (priority)
Projects
None yet
4 participants