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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use esbuild supported feature #8665

Merged
merged 8 commits into from Jun 28, 2022
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jun 19, 2022

Description

Use esbuild supported feature to explicitly enable "dynamic imports" and "import.meta" as supported features, regardless of target as Vite handles them.

This correctly fixes #5270 in case the user set a lower build.target version.

Additional context

Let me know if there's more supported features that I've missed 馃槵

In the second commit, I made an optimization to use modules target for dev optimizer, instead of build.target. This was changed in the past at #5095, but we discussed that it may be better to not rely on build.target in dev as it's not necessary to down-level them like in prod.

I'm also open to discussion on this part.


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

patak-dev
patak-dev previously approved these changes Jun 19, 2022
@sapphi-red
Copy link
Member

rollup supports top-level await (rollup/rollup#2235, rollup/rollup#3089) bundling from 1.29.0.
So setting 'top-level-await': true will enable users to use top-level await. (currently esbuild throws "Top-level await is not available in the configured target environment")

@sapphi-red
Copy link
Member

I think this test needs to set esbuild.supported because esbuild removed force polyfilling object spread syntax in v0.14.46.

// Force esbuild spread helpers (https://github.com/evanw/esbuild/issues/951)
console.log({ ...'foo' })

test('umd', async () => {
expect(await page.textContent('.umd')).toBe('It works')
const code = readFile('dist/my-lib-custom-filename.umd.js')
// esbuild helpers are injected inside of the UMD wrapper
expect(code).toMatch(/^\(function\(/)
})
test('iife', async () => {
expect(await page.textContent('.iife')).toBe('It works')
const code = readFile('dist/my-lib-custom-filename.iife.js')
// esbuild helpers are injected inside of the IIFE wrapper
expect(code).toMatch(/^const MyLib=function\(\){"use strict";/)
})

@DylanPiercey
Copy link
Contributor

Since esbuild is seemingly used for SSR now, shouldn't this list be configured for the node version for SSR?
I ran into the same issue as @sapphi-red with top level await, however the code was SSR only code that would be running in node anyway.

Also ideally wouldn't this pick up on a browserslist config for the default env and do something like: https://github.com/nihalgonsalves/esbuild-plugin-browserslist

@netlify
Copy link

netlify bot commented Jun 23, 2022

Deploy Preview for vite-docs-main ready!

Name Link
馃敤 Latest commit 34d53fe
馃攳 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62b879883e09910008b769e2
馃槑 Deploy Preview https://deploy-preview-8665--vite-docs-main.netlify.app
馃摫 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bluwy
Copy link
Member Author

bluwy commented Jun 23, 2022

Thanks for the explanation re top-level-await. I've added it, and also fix the lib test for iife and umd.

Since esbuild is seemingly used for SSR now, shouldn't this list be configured for the node version for SSR?
I ran into the same issue as @sapphi-red with top level await, however the code was SSR only code that would be running in node anyway.

The code change here would affect both browser and SSR, so I think it would cover your usecase too.

Also ideally wouldn't this pick up on a browserslist config for the default env and do something like: https://github.com/nihalgonsalves/esbuild-plugin-browserslist

Not sure if Vite would ever have first-class browserslist support, but currently we're setting our own baseline modules target as the default for dev and build. And the idea is to use plugin-legacy if you need to support older browsers.

@bluwy bluwy added the p2-nice-to-have 馃嵃 Not breaking anything but nice to have (priority) label Jun 23, 2022
@sapphi-red
Copy link
Member

sapphi-red commented Jun 23, 2022

rollup supports top-level await (rollup/rollup#2235, rollup/rollup#3089) bundling from 1.29.0.
So setting 'top-level-await': true will enable users to use top-level await. (currently esbuild throws "Top-level await is not available in the configured target environment")

Sorry I thought rollup removes top-level await after bundle but I noticed that I overlooked the preserved await. 馃檱
https://stackblitz.com/edit/vitejs-vite-blh1nx?file=dist%2Fassets%2Findex.6f8ca63b.js&terminal=dev

Thanks for fixing the test!

@bluwy bluwy mentioned this pull request Jun 24, 2022
7 tasks
sapphi-red
sapphi-red previously approved these changes Jun 24, 2022
@patak-dev patak-dev merged commit 2061d41 into vitejs:main Jun 28, 2022
@bluwy bluwy deleted the use-esbuild-supported branch June 28, 2022 07:11
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Optimized dependencies in 2.6.x cannot access import.meta
5 participants