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

Sentry reporting syntax error from IE11 (questions regarding bundling and package.json fields) #52

Closed
saulhardman opened this issue Sep 11, 2020 · 5 comments

Comments

@saulhardman
Copy link

Sentry is reporting an error that's being triggered in IE11 from a component bundled using this template. It's a syntax error and the offending code is below (note the arrow function in createInjector):

const isOldIE = typeof navigator !== 'undefined' &&
    /msie [6-9]\\b/.test(navigator.userAgent.toLowerCase());
function createInjector(context) {
    return (id, style) => addStyle(id, style);
}

The quick-fix for this is to configure rollup-plugin-vue to use ES5-compliant bundles for the normalizer and syleInjector:

// (other options omitted)
vue({
  normalizer: '~vue-runtime-helpers/dist/normalize-component.js',
  styleInjector: '~vue-runtime-helpers/dist/inject-style/browser.js',
}),

However, this led to me reviewing the logic behind how the bundles themselves are built and also which bundles are assigned to which fields in the package.json file.

I noticed that the "browser" field in package.json points to the *.esm.js bundle. However, you're explicitly removing support for "legacy" browsers for that build.

It's my understanding that tools such as Webpack will consume this "browser" bundle for browser-targeted bundles and will not transpile them unless explicitly instructed to do so (which is unlikely).

To resolve this issue whilst keeping the build configuration as it is, you could tell people authoring components using this template to inform their users that they'll need to transpile the bundles as part of their own build process (if their browser support requires it).

The other option is to create 2 ESM builds: dist/*.esm-modern.js which is assigned to the "module" field in package.json and dist/*.esm-legacy.js which is assigned to "browser". The legacy bundle would include ie > 10 in its Browserslist config and the above fix regarding rollup-plugin-vue.

Again, if my understanding is correct, the "module" bundle is intended to be used (currently) by tools such as Snowpack to enable development builds that utilise ESM. That means that the *.esm-modern.js bundle could also define its @babel/preset-env targets to be much more modern e.g. last 2 chrome versions, last 2 firefox versions.

Does that make sense? Am I right in my thinking?

@mgdodge
Copy link
Collaborator

mgdodge commented Sep 11, 2020

I have an update pending for the vuejs.org cookbook which aims to explain the main/module/browser fields, among other things. Just when I thought this was all figured out...

With Vue 3 on the horizon, and with that an official end-of-life for all IE support in vue, it's frustrating that things like this ruin what would otherwise be a "simple" recipe/utility. Even Microsoft themselves don't recommend using IE for browsing the web anymore!

It's not an uncommon occurrence that non-ES5 modules are published to npm - in fact, some pretty common packages are in that camp (D3 comes to mind). True, webpack must be instructed to transpile ES6 node modules, but at least that is opt-in. Transpiling the existing 'browser' build to ES5 means it's no longer a true esm build, and it passes the download cost to all users regardless - browsers that can handle ES6 are served older code, because that's all that what webpack picked up, even though the website owner might not even include IE in their target audience! Not to mention that ES5 builds are often quite a bit larger, so developers also pay a price in a larger download - not as much of a concern, but still true.

In the interest of moving the web forward, perhaps the utility could generate a sample README that includes information about adding IE support? This places the decision of cost where it belongs - with those using the module in their sites, who know the target audience and whether it is even necessary.

@saulhardman
Copy link
Author

saulhardman commented Sep 11, 2020

Hey @mgdodge, thanks for the response. We are indeed in an unusual Limbo right now.

Transpiling the existing 'browser' build to ES5 means it's no longer a true esm build

If you configure Rollup with { output: { format: 'esm' } }, but include ie > 10 (or a similar query) in the Browserlist config, the output will still be an ES module – it's just that the code exported by that module will be compatible with ES5. Bundlers then do the work of magicking away the ES module code so that the final bundled output is compatible with all browsers. This means it's possible to leverage tree-shaking and other benefits of ES modules, but still with universal support.

Side note about tree-shaking

This template makes tree-shaking impossible due to the dynamic assignment of the install function onto the component module (potential side effects that bundlers can't know about). This probably isn't an issue for 99% of the ways this template is used, but I wanted to export a helper util as a named export from the package in an environment (critical JavaScript in the <head>) where I needed the unused components to be tree-shaken away. I can see this was discussed in #39. It's not relevant to my use cases that components can be installed via Vue.use and so I've removed this line and now tree-shaking works as expected. (I'm putting this here for posterity's sake, I suppose.)

Good points regarding shipping code that's transpiled for compatibility with the lowest-common denominator, but that's also in the spirit of the web I think. The "module" and "browser" fields provide a great opportunity to offer 2 distinct 'modern' and 'legacy' bundles here. I think the "browser" field should always contain transpiled, polyfilled (in most cases) production code that works in a wide range of browsers. The "module" field should then target more modern environments (that have native support for ES modules) and by shipping the source .vue SFC, we're leaving things open for users to really fine-tune the build process, should they wish.

Thanks for all the work on this repository btw, it's been a great source of guidance 👍

@mgdodge
Copy link
Collaborator

mgdodge commented Sep 30, 2020

@saulhardman I am still not convinced that this utility should default to creating es5 code where it is not needed by everyone. As your initial report indicated, the createInjector code itself is not IE friendly. This means that the decision to support IE is opt-in from the 'official' tooling (intentionally or not), and thus should be opt-in for users of this utility as well. I am more comfortable with generating a README with instructions so that it is called out. With Vue 3 now released, there is a definite end-of-life for IE support in the vue ecosystem now.

Regarding tree shaking - I can make a small update to the 'single component' scaffolding that fixes the dynamic install function you pointed out, that is genuinely something that can be fixed. The problem does not exist when using the 'library' scaffolding. The change would probably land in the next major release, which will be landing shortly and gives the option of scaffolding Vue 2 and Vue 3 components.

Speaking of which, Vue 3 components, as currently transpiled by rollup-plugin-vue@next, are not tree-shakeable at all. And it's a lot more than just the one function causing the problem. When I get a bug report written for it, I will link to it from here.

@mgdodge
Copy link
Collaborator

mgdodge commented Oct 31, 2020

As promised, the tree-shaking issue I have logged with rollup-plugin-vue for Vue 3 components.

vuejs/rollup-plugin-vue#401

Also, I have published v4.0.0-beta.1 of this utility, which fixes the dynamic install function issue - please confirm if it works for you. When exporting more than one thing, even if you are exporting a single component plus a helper function, it might be considered a "library" at that point, FWIW.

@mgdodge
Copy link
Collaborator

mgdodge commented Jul 15, 2022

The decision has been made to deprecate vue-sfc-rollup in favor of better tooling in the Vue ecosyste, specifically the vite "library build" configuration. It is an excellent project, which is capable of everything vue-sfc-rollup strived to accomplish, and more.

@mgdodge mgdodge closed this as completed Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants