Polyfills #173

constgen · 2017-04-11T07:44:04Z

The problem is that 'babel-preset-env' doesn't consider defined platforms to filter only necessary polyfills. This change fixes this. Also polyfills are extracted to the 'vendor' chank.
Before

After

From 247KB down to 89KB

…ebug log it is already included

eliperelman

This is awesome! Just a few tweaks, and we should be able to ship this.

eliperelman · 2017-04-11T14:05:19Z

packages/neutrino-preset-web/index.js

@@ -85,8 +84,10 @@ module.exports = (neutrino) => {
    .when(process.env.NODE_ENV !== 'test', () => neutrino.use(chunk))
    .target('web')
    .context(neutrino.options.root)
+	 .entry('vendor')


Dedent this line so it lines up with .context().

Ok, will do

eliperelman · 2017-04-11T14:05:38Z

packages/neutrino-preset-web/polyfills.js

@@ -0,0 +1 @@
+import 'babel-polyfill';


Add a blank line to the end of this file.

My bad. Usually VSCode helps me with this automatically.

eliperelman · 2017-04-11T14:06:40Z

packages/neutrino-preset-web/index.js

@@ -67,14 +67,13 @@ module.exports = (neutrino) => {
  neutrino.use(htmlTemplate, neutrino.options.html);
  neutrino.use(namedModules);
  neutrino.use(compileLoader, {
-    include: [neutrino.options.source, neutrino.options.tests],
+    include: [neutrino.options.source, neutrino.options.tests, require.resolve('./polyfills.js')],


Do we have to add this file to the compilation? Does it work if we switch the polyfills file to use require?

require('babel-polyfill');

If you mean to avoid './polyfills.js' it will not work. babel-preset-env just want to see some where the importing of 'babel-polyfill'. And it doesn't see it if this file is an entry point itself.

I mean, if they want to not have polyfills in their project at all. They should have the ability to remove it if they wish.

To be honest didn't think about this "include". I see that it will not affect the build until somebody will import exactly this file ./polyfills.js.

eliperelman · 2017-04-11T14:10:29Z

packages/neutrino-preset-web/index.js

@@ -85,8 +84,10 @@ module.exports = (neutrino) => {
    .when(process.env.NODE_ENV !== 'test', () => neutrino.use(chunk))
    .target('web')
    .context(neutrino.options.root)
+	 .entry('vendor')
+      .add(require.resolve('./polyfills.js'))


Removing the polyfills file different, since we can't just remove based on the location of babel-polyfill. I'm not saying this is a bad thing, just different. We should probably add this to the docs, about how to remove the polyfills.

neutrino.config .entry('vendor') .delete(require.resolve('neutrino-preset-web/polyfills'))

Originally this refactoring had a separate entry point .entry('polyfills') which produced one more file chunk. In this case polyfills could be disabled easy by name. But I decided to make changes more soft and joined vendor with polyfills togehter. Now splitting vendor and polyfills seems not a bad idea from the configuration perspective.

eliperelman · 2017-04-11T14:12:08Z

@jaridmargolin any thoughts?

jaridmargolin · 2017-04-11T17:05:47Z

@eliperelman @constgen - The idea of creating a separate file that imports is definitely a creative solution that solves the problem.

I am curious if automatically including it in the vendor is correct. If I begin utilizing new es features in my index entry, I wouldn't want my vendor bundle cache to be busted (or if my supported browser set changes). Overall curious if vendors & pollyfills should be tightly coupled.

Originally this refactoring had a separate entry point .entry('polyfills') which produced one more file chunk. In this case polyfills could be disabled easy by name. But I decided to make changes more soft and joined vendor with polyfills togehter. Now splitting vendor and polyfills seems not a bad idea from the configuration perspective.

I think the the separate polyfills entry might be a better solution.

EDIT: I had previously watched a talk that spoke about the effects of single file vs multiple bundles which load in parallel. I want to dig a little deeper in how these decisions apply to first paint (on first visit vs revisit vs cache bust).

eliperelman · 2017-04-11T21:19:46Z

I'm leaning towards the separate polyfills entrypoint as well.

codecov · 2017-04-11T21:41:37Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #173   +/-   ##
=======================================
  Coverage   90.69%   90.69%           
=======================================
  Files          36       36           
  Lines         419      419           
=======================================
  Hits          380      380           
  Misses         39       39

Impacted Files	Coverage Δ
packages/neutrino-preset-web/index.js	`80.95% <ø> (ø)`	⬆️

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 f2c1dfb...9582366. Read the comment docs.

constgen · 2017-04-11T21:44:40Z

Made necessary changes. Now in my example I also added the 'lodash' library to entries and imported it in the index.js just to see how everything works together. The output is:

We still have an open question: "What if somebody will reconfigure a 'compile' rule 'include' section and erase require.resolve('./polyfills.js')?" Babel will no longer consider this file and the chunk size will rise again.

jaridmargolin · 2017-04-12T01:28:06Z

^ above I was referring to some findings here: https://youtu.be/6m_E-mC0y3Y?list=PLNYkxOF6rcICc687SxHQRuo9TVNOJelSZ&t=248

Its not an exact science but the overall takeaway is, in certain cases, making multiple requests in parallel can be comparable or even faster than requesting a single bundle. I think we could benefit from spending a tiny bit of time profiling performance before making a decision here.

* There are definitely additional resources on this topic but I had a difficult time finding them.

constgen · 2017-04-12T10:35:02Z

Additionally when we are in a dev mode (npm start) the rebuild will be faster because only index chunk is processed.

eliperelman · 2017-04-12T14:06:09Z

Tested this out locally, everything worked great!

eliperelman · 2017-04-12T14:37:40Z

Sooo, do we need to add polyfills to the chunks? Right now the only defaulted chunks from the chunk middleware are vendor and manifest. Thoughts?

constgen · 2017-04-12T15:22:50Z

Only if somebody is going to import/require polyfills to his code base. I believe it's not going to happen. But my opinion may be wrong.

eliperelman · 2017-04-12T15:29:37Z

I must've missed something, because when running this while using async, I received a message about regeneratorRuntime being missing. Modifying the web middleware to also include polyfill in the chunk seems to have fixed it.

constgen · 2017-04-12T17:36:17Z

Interesting. This is definitely related to one of commits in this branch where I removed the 'transform-regenerator' from the babel preset because it is included in the polyfills on demand.

constgen added 4 commits March 27, 2017 01:33

Rdesign Advanced Customization examples to use Neutrino v5 fashion

7366e4e

Merge branch 'master' of github.com:mozilla-neutrino/neutrino-dev

79ce01b

Fix polyfills set to match only supported platforms

6660bef

Remove 'tranform-regenerator' because according to the babel preset d…

e0b731e

…ebug log it is already included

eliperelman suggested changes Apr 11, 2017

View reviewed changes

constgen added 3 commits April 12, 2017 00:23

Fix indentation

09d069e

Add VSCode local settings to the ignore list

d97478c

Separate chunk for polyfills

9582366

eliperelman approved these changes Apr 12, 2017

View reviewed changes

eliperelman merged commit f6bcb7b into neutrinojs:master Apr 12, 2017

constgen deleted the polyfills branch April 12, 2017 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polyfills #173

Polyfills #173

constgen commented Apr 11, 2017

eliperelman left a comment

eliperelman Apr 11, 2017

constgen Apr 11, 2017

eliperelman Apr 11, 2017

constgen Apr 11, 2017

eliperelman Apr 11, 2017

constgen Apr 11, 2017

eliperelman Apr 11, 2017 •

edited

Loading

constgen Apr 11, 2017 •

edited

Loading

eliperelman Apr 11, 2017

constgen Apr 11, 2017

eliperelman commented Apr 11, 2017

jaridmargolin commented Apr 11, 2017 •

edited

Loading

eliperelman commented Apr 11, 2017

codecov bot commented Apr 11, 2017 •

edited

Loading

constgen commented Apr 11, 2017 •

edited

Loading

jaridmargolin commented Apr 12, 2017

constgen commented Apr 12, 2017

eliperelman commented Apr 12, 2017

eliperelman commented Apr 12, 2017

constgen commented Apr 12, 2017

eliperelman commented Apr 12, 2017

constgen commented Apr 12, 2017 •

edited

Loading

Polyfills #173

Polyfills #173

Conversation

constgen commented Apr 11, 2017

eliperelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

constgen Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman commented Apr 11, 2017

jaridmargolin commented Apr 11, 2017 • edited Loading

eliperelman commented Apr 11, 2017

codecov bot commented Apr 11, 2017 • edited Loading

Codecov Report

constgen commented Apr 11, 2017 • edited Loading

jaridmargolin commented Apr 12, 2017

constgen commented Apr 12, 2017

eliperelman commented Apr 12, 2017

eliperelman commented Apr 12, 2017

constgen commented Apr 12, 2017

eliperelman commented Apr 12, 2017

constgen commented Apr 12, 2017 • edited Loading

eliperelman Apr 11, 2017 •

edited

Loading

constgen Apr 11, 2017 •

edited

Loading

jaridmargolin commented Apr 11, 2017 •

edited

Loading

codecov bot commented Apr 11, 2017 •

edited

Loading

constgen commented Apr 11, 2017 •

edited

Loading

constgen commented Apr 12, 2017 •

edited

Loading