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

fix: upgrade esbuild to 0.20.x #16062

Merged
merged 2 commits into from
Mar 1, 2024
Merged

fix: upgrade esbuild to 0.20.x #16062

merged 2 commits into from
Mar 1, 2024

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Feb 29, 2024

Description

Upgrades esbuild to 0.20.x

Quite a few of our peers have already upgraded, so it is leading to us having a fair amount of duplicate installs of esbuild in our dependency tree. would be nice to bump the version here so we can de-dupe

Let me know if there's any reason you think we can't do this yet


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, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Feb 29, 2024

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

@sapphi-red
Copy link
Member

I didn't make a PR to bump esbuild because we don't bump esbuild minors in a patch and Vite 4.1.0 was already kinda RC phase
when esbuild 0.20 was released. But it seems there's no breaking change that affects Vite. I think we can release this PR in a patch.

@sapphi-red sapphi-red added dependencies Pull requests that update a dependency file p3-minor-bug An edge case that only affects very specific usage (priority) labels Mar 1, 2024
@sapphi-red sapphi-red changed the title chore: upgrade esbuild to 0.20.x fix: upgrade esbuild to 0.20.x Mar 1, 2024
@43081j
Copy link
Contributor Author

43081j commented Mar 1, 2024

that makes sense, does seem like we should be ok per the changelog at least

thanks for taking a look 👍

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

@patak-dev patak-dev merged commit 899d9b1 into vitejs:main Mar 1, 2024
10 checks passed
@bluwy
Copy link
Member

bluwy commented Mar 1, 2024

Hmm I feel like we should have waited this for the next minor since this breaking change feels a bit risky:

Reorder implicit file extensions within node_modules

@patak-dev
Copy link
Member

I'm fine with reverting it and wait 👍

@sapphi-red
Copy link
Member

sapphi-red commented Mar 1, 2024

The reorder behavior already existed in 0.18. The reorder change in 0.20 small IMO.

Specifically the default implicit file extension order is .tsx,.ts,.jsx,.js,.css,.json which used to become .jsx,.js,.css,.json,.tsx,.ts in node_modules directories. With this release it will now become .jsx,.js,.tsx,.ts,.css,.json instead.

@bluwy
Copy link
Member

bluwy commented Mar 1, 2024

I think re-ordering extensions is a big deal though, it changed again in 0.20 which isn't the same as 0.18. This affects especially for internal projects where packages are published raw, and projects where they set esbuildOptions.resolveExtensions and now js/ts work differently. A patch usually involves bug fixes / improvements, but this seems like a bigger behaviour change.

@sapphi-red
Copy link
Member

Just in case, the change in 0.20 doesn't swap the order of .js, .ts. The change affects when you have both a.css and a.js and you import it with import './a' (pre-0.20: resolves to a.css, 0.20+: resolves to a.js).
But yeah, I agree that it's not that small. Let's revert it and wait for 4.2.

sapphi-red added a commit that referenced this pull request Mar 1, 2024
@43081j
Copy link
Contributor Author

43081j commented Mar 1, 2024

in terms of getting this done in 4.2, when/how do we re-merge it?

happy to remember to push it again or rework the PR if i understand where we want it

@sapphi-red
Copy link
Member

Thank you. But I'll create the reapply PR when the revert PR is merged, since that should be possible with a single button on GitHub. I'll put it in the milestone so that we don't forget to merge it in 4.2, too.

@bluwy
Copy link
Member

bluwy commented Mar 4, 2024

Thanks for reverting this Sapphi 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants