Skip to content

fix(plugin-legacy): replace esbuild-plugin-browserslist with browserslist-to-esbuild#15988

Merged
patak-cat merged 2 commits intovitejs:mainfrom
btea:chore/add-esbuild-dep
Feb 21, 2024
Merged

fix(plugin-legacy): replace esbuild-plugin-browserslist with browserslist-to-esbuild#15988
patak-cat merged 2 commits intovitejs:mainfrom
btea:chore/add-esbuild-dep

Conversation

@btea
Copy link
Contributor

@btea btea commented Feb 20, 2024

Description

When I execute pnpm i the following warning message appears.

image

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, 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.

@bolt-new-by-stackblitz
Copy link

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

@bluwy
Copy link
Member

bluwy commented Feb 21, 2024

Maybe we should switch to https://www.npmjs.com/package/browserslist-to-esbuild instead to avoid that peer dep. The API we're using shouldn't need to use esbuild, so I think we should avoid depending on esbuild if possible to prevent potential version mismatch and unnecessarily occupying disk space.

@bluwy bluwy added plugin: legacy p2-nice-to-have Not breaking anything but nice to have (priority) labels Feb 21, 2024
@btea btea changed the title chore(dep): plugin-legacy add esbuild dep chore(dep): replace esbuild-plugin-browserslist with browserslist-to-esbuild Feb 21, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks!

@bluwy bluwy changed the title chore(dep): replace esbuild-plugin-browserslist with browserslist-to-esbuild fix(plugin-legacy): replace esbuild-plugin-browserslist with browserslist-to-esbuild Feb 21, 2024
@patak-cat patak-cat merged commit 37af8a7 into vitejs:main Feb 21, 2024
@btea btea deleted the chore/add-esbuild-dep branch February 21, 2024 13:37
@patak-cat
Copy link
Member

It seems this PR broke several downstream CIs:

FAIL  playground/vue-legacy/__tests__/vue-legacy.spec.ts [ playground/vue-legacy/__tests__/vue-legacy.spec.ts ]
  Error: require() of ES Module /home/runner/work/vite-ecosystem-ci/vite-ecosystem-ci/workspace/vite-plugin-vue/vite-plugin-vue/node_modules/.pnpm/browserslist-to-esbuild@2.1.1_browserslist@4.23.0/node_modules/browserslist-to-esbuild/src/index.js from /home/runner/work/vite-ecosystem-ci/vite-ecosystem-ci/workspace/vite-plugin-vue/vite-plugin-vue/node_modules/.pnpm/file+..+..+vite+packages+plugin-legacy_vite@5.1.4/node_modules/@vitejs/plugin-legacy/dist/index.cjs not supported.
  Instead change the require of index.js in /home/runner/work/vite-ecosystem-ci/vite-ecosystem-ci/workspace/vite-plugin-vue/vite-plugin-vue/node_modules/.pnpm/file+..+..+vite+packages+plugin-legacy_vite@5.1.4/node_modules/@vitejs/plugin-legacy/dist/index.cjs to a dynamic import() which is available in all CommonJS modules.

Example: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/8005338357/job/21864542117

@btea let us know if you have an idea to fix this, if not I think we should revert it for now.

@bluwy
Copy link
Member

bluwy commented Feb 22, 2024

It looks like browserslist-to-esbuild is ESM only, I think we need to dynamically import it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2-nice-to-have Not breaking anything but nice to have (priority) plugin: legacy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants