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

refactor!: esbuild 0.11 upgrade, remove dynamic-import-polyfill #2976

Merged
merged 1 commit into from
May 5, 2021

Conversation

ludofischer
Copy link
Contributor

Description

Upgrade esbuild to the latest version, so vite stays in sync with the experience users get when they install esbuild standalone (e.g. npm install esbuild).

Additional context

There are some interesting bug fixes for JavaScript transpilation (for example evanw/esbuild#1131, evanw/esbuild#977, evanw/esbuild#959, evanw/esbuild#1066), but the upgrade should be relatively safe as most esbuild changes only affect chunking and code splitting (while vite uses rollup for that).


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 Apr 13, 2021
@Shinigami92 Shinigami92 added the dependencies Pull requests that update a dependency file label Apr 14, 2021
@Shinigami92 Shinigami92 requested a review from antfu April 14, 2021 07:05
@Shinigami92
Copy link
Member

I don't add p1-chore intentionally, cause this can have huge side-effects and there were many breaking changes in esbuild.

@patak-dev patak-dev dismissed their stale review April 14, 2021 07:50

There are issues with dynamic imports after upgrading

@patak-dev
Copy link
Member

Thanks for the PR @ludofischer. We didn't update until now because there were breaking changes in esbuild 0.10 that affect Vite. It looks like the test suite cannot detect some issues that appear after bumping esbuild to 0.11.10.

You can yarn link to see the issues when building https://github.com/antfu/vitesse for example. dynamic imports end up as require in the bundled code. In case you want to dig deeper into this, we have a #contributing channel in Vite Land to discuss.

@egoist
Copy link
Contributor

egoist commented Apr 17, 2021

Seems it's caused by the edge18 target, not sure how to fix this other than increasing the version number to 79.

Related evanw/esbuild#1146, evanw/esbuild#1084 (maybe @evanw can help) More context: Vite uses a dynamic import polyfill for browsers that don't support it natively.

@GrygrFlzr
Copy link
Member

According to Microsoft, legacy Edge has been EOL since last month on March 9th, 2021 and the new Edge was added during a security update on April 13th, 2021, which includes the removal of legacy Edge.

@egoist
Copy link
Contributor

egoist commented Apr 17, 2021

Nice timing, changing it to edge79 sounds great then.

@patak-dev
Copy link
Member

@GrygrFlzr @egoist were you able to avoid the dynamic import issue in a vitesse app when by moving to edge79 target? I tried locally but couldn't make it work (for support in workers, it looks like we need edge80, but I still see the same issue)

@nihalgonsalves
Copy link
Member

@patak-js Tried it out and cross-referenced the DynamicImport config for esbuild, and this config works with Vitesse (no require in the build output):

  // handle special build targets
  if (resolved.target === 'modules') {
    // https://caniuse.com/usage-table
    // https://caniuse.com/es6-module
    // https://caniuse.com/es6-module-dynamic-import
    // https://github.com/evanw/esbuild/blob/d354bbb/internal/compat/js_table.go#L87
    resolved.target = [
      'es2019',
      // previously: edge18
      'edge79',
      // previously: firefox60
      'firefox67',
      // previously: chrome61
      'chrome63',
      // previously: safari11
      'safari11.1'
    ]
  } else if (resolved.target === 'esnext' && resolved.minify !== 'esbuild') {
    // esnext + terser: limit to es2019 so it can be minified by terser
    resolved.target = 'es2019'
  }

However, with the import polyfill we'd actually support earlier versions as well. It's a shame we can't just pass "chromeX,safariX,...,DynamicImport" to esbuild...

@ludofischer
Copy link
Contributor Author

@patak-js Tried it out and cross-referenced the DynamicImport config for esbuild, and this config works with Vitesse (no require in the build output):

Do I understand correctly that this change moves support for some browsers out of vanilla vite and to the legacy plugin?

  1. setting the target explicitly to a an older browser like edge20 would still generate broken require() bundles. So you would need to use @vitejs/plugin-legacy for those

  2. on a positive node, the number of possible dynamic loaders would reduce from 3 to 2 (since all browsers that receive the ESM bundle also support dynamic import(), you could completely remove the dynamic import() polyfill which would leave only fully native ESM and SystemJS )

@nihalgonsalves
Copy link
Member

nihalgonsalves commented Apr 27, 2021

Do I understand correctly that this change moves support for some browsers out of vanilla vite and to the legacy plugin?

It depends on what the real feature diff is between the changed browser versions. We need to look at the esbuild compatibility table and see what other features are considered "supported" by esbuild due to the change. If the only significant change is dynamic imports, then we'd retain the same supported versions due to the default polyfill.

If not, then we have two options, presumably:

  • Consider it a breaking change, since users who need to support browsers included in the older target spec would now have to add the legacy plugin. We'd be able to remove the dynamic import polyfill in this case.
  • Add more automatic polyfills (perhaps an automatic legacy plugin config) for the newly required features, retaining compatibility.

@ludofischer
Copy link
Contributor Author

ludofischer commented Apr 27, 2021

I've given it a shot to see what updating the browser targets and removing the dynamic import polyfill looks like.

  • I feel like it breaks too much if the only gain is using a newer esbuild, but according to https://caniuse.com/usage-table the change does not drop many users (especially if Edge 18 is EOL).
  • this pull request misses a fix for the tests so they are able to detect the original build failure with esbuild 0.11 (packages/playground/dynamic-import/ breaks if I run it manually with vite preview but somehow the tests pass even in build mode).

add more automatic polyfills (perhaps an automatic legacy plugin config) for the newly required features, retaining compatibility

I would not like the legacy plugin turning on automatically, as I would find the build time and output change jarring.

@nihalgonsalves
Copy link
Member

I feel like it breaks too much if the only gain is using a newer esbuild, but according to caniuse.com/usage-table the change does not drop many users (especially if Edge 18 is EOL).

As a counter-point: there have been some important improvements since the current esbuild version, especially [email protected], which should improve compatibility with old packages or non-standard packages significantly.

@ludofischer ludofischer changed the title fix: upgrade esbuild to 0.11 upgrade esbuild to 0.11 Apr 27, 2021
@patak-dev
Copy link
Member

Thanks for looking into this @nihalgonsalves! We really need to move to esbuild 0.11.x (and forward). There are many issues that are going to be solved after this. I think that bumping the browser versions for this is ok, we don't have many options.
We will discuss with Evan what is the best strategy to merge this. I think it would be a minor.

Note: There have already been other releases where we needed to bump the browser versions, see #2566

@nihalgonsalves
Copy link
Member

nihalgonsalves commented Apr 27, 2021

@patak-js yup, I gave it some more thought, and I think that we could update the Browser Compatibility section to refer to a browserslist query instead.

# for example:
$ npx browserslist "defaults and supports es6-module, not opera > 0, not samsung > 0, not and_qq > 0"
# returns this:
and_chr 90
and_ff 87
android 90
chrome 90
chrome 89
chrome 88
chrome 87
edge 90
edge 89
edge 88
firefox 88
firefox 87
firefox 86
firefox 78
ios_saf 14.5
ios_saf 14.0-14.4
ios_saf 13.4-13.7
safari 14
safari 13.1

which would let us use a target of ['es2019', 'edge88', 'firefox78', 'chrome87', 'safari13.1'] and let us iterate quicker in the future. Users who want to support browsers outside of this (or another specified range) would know that they have to use the legacy plugin.

@patak-dev
Copy link
Member

@nihalgonsalves but the dynamic import polyfill gives us a wider range of browsers than that query, no?

@nihalgonsalves
Copy link
Member

nihalgonsalves commented Apr 27, 2021

Yeah, it's just an example. I mean that we should document some range instead of exact browser versions in the README.

Though the fact that the default query (npx browserslist) returns these relevant lowest versions:

edge 88, chrome 87, firefox 78, safari 13.1

is probably a sign that we don't really need to support earlier versions. (The query is equivalent to > 0.5%, last 2 versions, Firefox ESR, not dead, see here)

@patak-dev
Copy link
Member

I think that is a good idea. Is there a way to add the dynamic import polyfill coverage to a browserlists query?

@nihalgonsalves
Copy link
Member

nihalgonsalves commented Apr 27, 2021

I don't think so. But by usage, browsers that don't support dynamic imports are actually a very minuscule percentage now, that's the argument @ludofischer and I were trying to make - we could just drop the polyfill.

@patak-js A better way to illustrate it: es6-module @ 93.07% vs es6-module-dynamic-import @ 92.79%. We already require the first, so the difference with upgrading to the default browserslist range and dropping the polyfill is 0.28% usage-relative

@ludofischer ludofischer force-pushed the update-esbuild branch 2 times, most recently from f851eb3 to f5e859c Compare April 28, 2021 14:22
@patak-dev
Copy link
Member

@ludofischer we discussed with Evan and we can move forward with the approach proposed by @nihalgonsalves. 0.2% of browsers don't justify the complexity of the dynamic import polyfill (see #1512 for an example of an issue with the plugin). And on top of that, as @nihalgonsalves explained, being able to specify the browser query as a features query instead of hard numbers is better for our users, and to avoid issues if esbuild changes again.

Technically, this change would require a major change but the affected browser set is so small that we will need to do a tradeoff here. This change is going to be released in the next minor as Vite v2.3.0

@ludofischer could you also update the docs at: https://vitejs.dev/guide/#browser-support and here https://vitejs.dev/guide/build.html#browser-compatibility? We can simplify this because pointing to full es6-module support. And add the new dynamic query

defaults and supports es6-module, not opera > 0, not samsung > 0, not and_qq > 0

Also, we should remove the application of the dynamic import polyfill by default but we should leave the option so we don't error out because of its presence in vite.config.js. We could add a deprecation message if the option is present pointing to the use of @vitejs/plugin-legacy.
The import polyfill file should also remain in the package because there are some users that are importing it by hand. We could also add a deprecation warning if this file is loaded, or just leave the file there for the foreseeable future.

Thanks again for the work on this PR.

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.

@ludofischer
Copy link
Contributor Author

defaults and supports es6-module, not opera > 0, not samsung > 0, not and_qq > 0

Don't you mean es6-module-dynamic-import instead of es6-module?

@nihalgonsalves
Copy link
Member

nihalgonsalves commented May 1, 2021

I believe both are irrelevant, as they don't change the output of the defaults query.

We could keep it simple:

## Browser Compatibility

-The production bundle assumes a baseline support for modern JavaScript. By default, all code is transpiled targeting [browsers with native ESM script tag support](https://caniuse.com/es6-module):
+The production bundle assumes a baseline support for modern JavaScript. Vite targets [browserslist](https://github.com/browserslist/browserslist)'s `defaults` query.
+
+This is a moving target based current usage. Run `npx browserslist` to see the current versions.

-- Chrome >=61
-- Firefox >=60
-- Safari >=11
-- Edge >=16

-A lightweight [dynamic import polyfill](https://github.com/GoogleChromeLabs/dynamic-import-polyfill) is also automatically injected.

You can specify custom targets via the [`build.target` config option](/config/#build-target), where the lowest target is `es2015`.

Note that by default, Vite only handles syntax transforms and **does not cover polyfills by default**. You can check out [Polyfill.io](https://polyfill.io/v3/) which is a service that automatically generates polyfill bundles based on the user's browser UserAgent string.

Legacy browsers can be supported via [@vitejs/plugin-legacy](https://github.com/vitejs/vite/tree/main/packages/plugin-legacy), which will automatically generate legacy chunks and corresponding ES language feature polyfills. The legacy chunks are conditionally loaded only in browsers that do not have native ESM support.

We could still document the current versions, but the docs would go out of date constantly.

antfu
antfu previously approved these changes May 5, 2021
nihalgonsalves
nihalgonsalves previously approved these changes May 5, 2021
Copy link
Member

@nihalgonsalves nihalgonsalves left a comment

Choose a reason for hiding this comment

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

LGTM

@patak-dev
Copy link
Member

@ludofischer looks like there are conflicts with main, seems that we can merge this once they are resolved 👍🏼

BREAKING CHANGE: update modules target to avoid mixing `import` and `require()`

BREAKING CHANGE: remove dynamic import polyfill

docs: update browser support docs
@ludofischer
Copy link
Contributor Author

@ludofischer looks like there are conflicts with main, seems that we can merge this once they are resolved 👍🏼

I've rebased on main and resolved the conflict (it was // vs /* */ in a comment!).

@patak-dev patak-dev requested a review from antfu May 5, 2021 09:13
@patak-dev patak-dev changed the title refactor!: upgrade esbuild to 0.11, remove dynamic-import-polyfill refactor!: esbuild 0.11 upgrade, remove dynamic-import-polyfill May 5, 2021
@patak-dev patak-dev merged commit 05fd1e2 into vitejs:main May 5, 2021
@patak-dev
Copy link
Member

Thanks a lot for the work on this PR 🙌🏼

@ludofischer ludofischer deleted the update-esbuild branch May 5, 2021 14:07
sapphi-red added a commit to sapphi-red/vite that referenced this pull request May 18, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants