Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

WIP 2946 Switch to using babel-preset-env #2987

Closed
wants to merge 5 commits into from

Conversation

fzzzy
Copy link
Contributor

@fzzzy fzzzy commented Oct 24, 2017

The site now has 0 javascript errors on IE 11! (It still looks ugly though, because IE doesn't do flexbox properly)

@codecov-io
Copy link

codecov-io commented Oct 24, 2017

Codecov Report

Merging #2987 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2987   +/-   ##
=======================================
  Coverage   84.12%   84.12%           
=======================================
  Files         103      103           
  Lines        2708     2708           
=======================================
  Hits         2278     2278           
  Misses        430      430

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28034c...ad7d508. Read the comment docs.

@fzzzy
Copy link
Contributor Author

fzzzy commented Oct 25, 2017

From testing this a little more, it looks like the selective inclusion does not work unless I comment out the vendor file again. If the bundling puts common chunks into vendor, I see all of core-js and regenerator-runtime being included. If the bundling uses only a single app.js file, I see only core-js/modules and regenerator-runtime being included.

@fzzzy
Copy link
Contributor Author

fzzzy commented Oct 25, 2017

If I remove Internet Explorer, the app.js bundle gets smaller, so the selective inclusion is definitely working, but only if we don't split our bundle into app.js and vendor.js.

@fzzzy fzzzy changed the title 2946 Switch to using babel-preset-env WIP 2946 Switch to using babel-preset-env Oct 26, 2017
@fzzzy
Copy link
Contributor Author

fzzzy commented Oct 26, 2017

Marking this as WIP because as it is, all of core-js is getting included in vendor. We need some more discussion about the app/vendor split.

Also, if we include Internet Explorer support, the site does work, but looks ugly due to IE not supporting flexbox correctly. I think we should drop IE support, but again, needs discussion.

@fzzzy
Copy link
Contributor Author

fzzzy commented Oct 26, 2017

app + vendor on production is about 624k. This PR as it is (which includes all of core-js) is almost 2 megs. If I unify app + vendor but leave IE support, the bundle size is still almost 2 megs. If I remove IE support and set chrome, firefox, safari, and edge to "last 2 versions" the bundle size drops to about 1.6 megs, so the polyfills we are missing on prod are about a meg. We do get Sentry errors which are caused by missing polyfills.

@chuckharmston
Copy link

chuckharmston commented Oct 26, 2017 via email

@lmorchard
Copy link
Contributor

lmorchard commented Oct 26, 2017

We need some more discussion about the app/vendor split.

The main benefit, IMO, is that the vendor.js bundle doesn't change unless dependencies change. So, if only site code changes, then we're only shipping a new app.js to users on site updates.

But, if maintaining a separate vendor.js is preventing us from shrinking bundle size overall - it's not really worth keeping around. As-is, it's a pretty brute force mechanism I just carried over from browserify: I just flag all the vendor modules from package.json and roll them in.

I think there are some way smarter things we can do webpack-wise to get a split-out vendor.js. But, that will take a little research I think. Might just be worth ditching vendor.js for now if it's easier to get a smaller single app.js with selective polyfills

@fzzzy
Copy link
Contributor Author

fzzzy commented Oct 26, 2017

@lmorchard Ok, I think the right thing to do for now is to get rid of vendor. At least then we will be moved to babel-preset-env and the polyfills that are included depend on our browser support matrix. At some point in the future, we can look into adding vendor back in with a more intelligent strategy.

However, adding the polyfills even for just the last two versions of Firefox, Chrome, Edge, and Safari adds a whole meg, erasing all the benefits of the work we did to shrink the bundle size. :(

We actually do see a few exceptions on production because we are missing polyfills:

  • fetch
  • "Array.includes is not a function"

It wouldn't be hard to switch these usages to XHR and indexOf. Then regenerator-runtime might be the only polyfill we would really need again.

However, I could not get useBuiltIns: "usage" to work, only useBuiltIns: true. So the polyfills that are included depend on our browser support matrix, not our actual usage of new features.

@lmorchard
Copy link
Contributor

We actually do see a few exceptions on production because we are missing polyfills:
fetch

This one should be fixed since I added the whatwg-fetch polyfill, but I don't think it's been deployed yet. That polyfill is only about 12k, which seems an okay tradeoff vs rewriting things

…fari, and Edge to try to get the bundle size down
@fzzzy
Copy link
Contributor Author

fzzzy commented Nov 21, 2017

I'm going to close this for now and reopen it later with a new branch that uses the babel-preset-env 2 beta.

@fzzzy fzzzy closed this Nov 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants