Babel MVP #143

Andarist · 2017-10-04T17:18:31Z

Proof of concept for #140 (comment)

Not yet ready to merge!

Extending an Error is an issue for babel - had to add for the time being custom transform for this. It's always possible to write buble-like class transform, shouldnt be a problem at all.

It should be easy to compare outputs (before and after) and discuss differences as you guys create "flat bundles" for the distribution

Andarist · 2017-10-04T17:19:10Z

packages/core/.babelrc

@@ -0,0 +1,5 @@
+{


in babel7 (soon-ish) it will be possible to use .babelrc.js directly and therefore omit those .babelrc entirely

Andarist · 2017-10-04T17:20:30Z

packages/disposable/.babelrc.js

+    }]
+  ],
+  plugins: [
+    ['transform-builtin-extend', {


the custom plugin for extending built-ins - as mentioned above

Andarist · 2017-10-04T17:22:30Z

package.json

@@ -16,6 +16,26 @@
  "author": "brian@hovercraftstudios.com",
  "license": "MIT",
  "devDependencies": {
-    "lerna": "^2.1.2"
+    "@briancavalier/assert": "^3.2.0",
+    "babel-core": "^6.26.0",


ive hoisted all used packages to the root package.json - according to lerna team this should result in faster bootstraps

Cool. I was under the impression that each package had to list its own deps and then use bootstrap --hoist. This is much better.

I believe that each pkg should still lost its own deps, at least i treat lerna packages being in single repository just some kimd of implementation detail. Ideally i always want to keep them separate so the pkg's dir could copied/moved and still keep working without changes.

Also each pkg need to have its deps with executables listed so they can be linked properly and used from npm scripts

Gathering all deps in root package.json is an additional step which according to lerna docs (i think) allows lerna for faster bootstraps but i aint sure why.

https://github.com/lerna/lerna/blob/master/FAQ.md#root-packagejson

The root can also hold all the "hoisted" packages, which speeds up bootstrapping when using the --hoist flag.

Andarist · 2017-10-04T17:23:34Z

package.json

+    "babel-plugin-external-helpers": "^6.22.0",
+    "babel-plugin-transform-builtin-extend": "^1.1.2",
+    "babel-preset-env": "^1.6.0",
+    "babel-preset-power-assert": "^1.0.0",


couldnt find if it was used before or if this is some leftover from the past, ive kept it for now but it seems this can be either removed or added to the used presets in prelude package

Oops. Yes, we can get rid of it.

TrySound · 2017-10-04T17:39:30Z

packages/core/.babelrc.js

+  presets: [
+    ['env', {
+      modules,
+      loose: true,


forceAllTransforms: true

was thinking about it, but aint sure if there is any benefit in that if we do not provide non-default targets, thoughts?

I think it's same target like uplify before. At least if defaults will be changed then nothing will be broken.

I think it's same target like uplify before.

Just a confirmation - that's exactly what it is

At least if defaults will be changed then nothing will be broken.

Good point, gonna add this later.

I meant semantically the same too.

Am I understanding forceAllTransforms correctly in that it essentially causes babel-preset-env to behave like babel-preset-es2015?

Kinda, its job is to output pure es5 which can be ofc understood by subsequent tools (like uglifyjs)

Preset2015 has only es2015 plugins in it, but preset-env has also plugins from other yearly presets and also it has (or soon it will, aint sure) shipped proposals plugins (like when browsers implement language features from stage 3 or something).

TrySound · 2017-10-04T17:42:28Z

Better to merge this first

Andarist · 2017-10-04T20:20:02Z

Sure thing, I just didn't want to wait before creating a PR, when the other one gets merged in I'm gonna rebase this one.

briancavalier · 2017-10-07T00:16:02Z

Thanks for working on this, @Andarist! Apologies for creating a rebase headache by merging #141.

Andarist · 2017-10-07T00:53:46Z

Dont worry, i knew it will be needed later from the very beginning, also shouldnt be particularly headache-y ;)

Andarist · 2017-10-07T09:17:02Z

Ok, I've rebased this PR, please keep in mind that this is not merge-ready, because of the usage of custom (non es5 compatible) babel-plugin-transform-builtin-extend. If you decide that you want to merge this PR, it should be rewritten first - my proposal is to just create buble-compatible class transform. Ofc I offer to write it 😉

davidchase · 2017-10-07T22:19:03Z

hi @Andarist this is quite interesting setup you have here, is there anyway to compare the results of this setup to our current setup to see what wins we get?

Andarist · 2017-10-07T23:02:08Z

The diff can be seen here. Unfortunately it is not easily comparable. I've studied it for some time and mostly (haha 🤣) the difference is in class transform:

buble generates a little bit simpler classes
buble handles built-in extension just fine (you extend Error), atm babel doesnt handle this too well
buble for simple cases doesnt wrap prototype pattern in an IIFE (it does it only when u use inheritance) - this is not a good thing, assignments to the unused function (such as prototype method assignment to the constructor) prevents dead code elimination
with babel you can use rich ecosystem of plugins (ive added one for adding #__PURE__ annotations), or write ur own

Ofc as mentioned above - ur original code style is lost in the output when using babel.

davidchase · 2017-10-09T01:58:52Z

any examples of bundle size comparisons ?

Andarist · 2017-10-09T07:37:46Z

Sure - I can check them out and report back later, but diffs will be a little bit off, because a PR is not finished due to the issues described regarding class transform.

I always measure such things by inspecting minified+gziped UMD bundles, is that ok or should I measure in some other way?

davidchase · 2017-10-09T12:19:27Z

I think that would be nice to see you could use https://github.com/mostjs/core/tree/master/examples/counter as a way to compare

briancavalier · 2017-10-11T00:51:16Z

I always measure such things by inspecting minified+gziped UMD bundles, is that ok

@Andarist Yep, that'd be just fine. Thank you for all your work on this so far! Looking forward to seeing size comparisons.

briancavalier · 2017-10-19T13:08:47Z

Ping @Andarist. We're definitely still interested in seeing a size comparison. Is there anything else we can do to help?

Andarist · 2017-10-19T13:57:06Z

Nah, not really - I will send comparisons as soon as I can, just couldn't focus at this PR lately as I had some other issues to tackle on.

briancavalier · 2017-10-19T14:04:13Z

Thanks @Andarist. No worries 😄

Andarist · 2017-10-21T10:36:46Z

All results for UMD minified and gzipped bundles

./packages/core/dist/mostCore.js
pre - 9.27 kB
post - 9.47 kB

./packages/disposable/dist/index.js
pre - 1.59 kB
post - 1.87 kB

./packages/prelude/dist/index.js
pre - 1.06 kB
post - 1.05 kB

./packages/scheduler/dist/index.js
pre - 3.3 kB
post - 3.31 kB

Difference is there, although quite small and as I've written before - alternative class transform should be written before merging this in, which potentially could decrease the those size differences.

I've also noticed that only the core package is building uglified bundle, other do not do that. Is it intended?

briancavalier · 2017-11-28T12:44:26Z

@Andarist Whew! Now that 1.0 is released, I'm going to spend some time on this. My plan is to build https://github.com/mostjs/core/tree/master/examples/counter using master and this branch so we can see the size difference in an application. Granted, it's a trivial application, but I think that's a good place to start since it should result in a fairly large size difference (at least, I think it should!). If it does then I think we'll feel good about moving ahead.

Andarist · 2017-11-28T12:58:19Z

Recent versions of rollup might be able to tree-shake some curry calls so actually #__PURE__ might help less there.

The experiment should use webpack with ModuleConcatenationPlugin on and uglifyjs, those settings should give better tree-shaking results - webpack alone actually has quite poor algorithm for this stuff.

I will try to rebase the branch a little bit later.

briancavalier · 2017-11-28T13:36:03Z

I made a couple local branches, one based on this PR branch (rebased to latest master), and one based on latest master. Both add rollup-plugin-uglify and uglify-es to the examples dir rollup config. I built the counter app in both branches, and verified that both still work correctly in Chrome, with no errors or warnings in the console.

Here are the results with just uglify, and with uglify+gzip:

❯ /bin/ls -l
total 168
-rw-r--r--  1 brian  staff  38834 Nov 28 08:13 index.base.js
-rw-r--r--  1 brian  staff   7792 Nov 28 08:24 index.base.js.gz
-rw-r--r--  1 brian  staff     89 Nov 28 08:18 index.js.map
-rw-r--r--  1 brian  staff  21925 Nov 28 08:18 index.pure.js
-rw-r--r--  1 brian  staff   4581 Nov 28 08:24 index.pure.js.gz

index.base.* is current master without PURE annotations, and index.pure.* is with PURE annotations. The non-gzip pure version is 21925 / 38834 = 56% of the size of the corresponding base version. The gzip pure version is 4581 / 7792 = 58% the size of the corresponding base version.

The savings are quite significant. Again, this is for a trivial app that uses a tiny amount of the total API surface area. Obviously, an app that uses more of the API will see less of a savings ... but that's the whole point: You get exactly what you opt into, and no more.

This seems compelling enough to me to proceed, but I'd love to hear everyone else's thoughts.

briancavalier · 2017-11-28T13:44:45Z

@Andarist Ok, thanks for the info about recent rollup. I'll try to run the experiment again with the latest version and post more results. We should do a similar test with webpack. The savings are probably less, but its worth seeing.

I have to switch to day job mode now, but I'll push my branches when I have a chance in case anyone else wants to experiment.

briancavalier · 2017-11-30T12:16:16Z

And there we go.

Next, I'm going to try to get rid of the need for extending Error. I'll do that in another PR so we can all see what it looks like before making a decision about it.

Andarist · 2017-11-30T12:16:58Z

👍 that would be a breaking change though, wouldnt it?

briancavalier · 2017-11-30T12:32:23Z

I think it depends on exactly what we do. The particular error thrown by disposeAll isn't part of the public API, so I feel it's ok to change it. Using native prototypal inheritance instead of extends may be a solution as well.

Andarist · 2017-11-30T12:43:15Z

Then both options seems fine to me, certainly better to use prototypal inheritance for a single use case than adding an additional (somewhat) complex plugin. If you follow that path it's advised to use IIFE to output a class into a single variable.

briancavalier · 2017-11-30T12:55:35Z

For comparison:

85031a7 uses prototypal inheritance. This seems pretty good to me.
7bcb378 uses a regular Error. This is slightly a "bigger" change. I don't see much advantage, but it also seems fine to me.

Andarist · 2017-11-30T13:20:24Z

Personally I like 1st better, seems like a cleaner change and being more straightforward about its purpose.

I'd only change it to:

export const DisposeAllError = (Error => {
  function DisposeAllError(message, errors) {
    Error.call(this, message)
    this.message = message
    this.name = DisposeAllError.name
    this.errors = errors

    if (Error.captureStackTrace) {
      Error.captureStackTrace(this, DisposeAllError)
    }

    this.stack = `${this.stack}${formatErrorStacks(this.errors)}`
  }	

  DisposeAllError.prototype = Object.create(Error.prototype)

  return DisposeAllError
})(Error)

briancavalier · 2017-12-01T13:09:53Z

@Andarist Agreed about the first option.

What's the reason for the IIFE in this particular case? It's not clear to me what it's defending against.

Andarist · 2017-12-01T13:28:38Z

Having this in top level scope:

DisposeAllError.prototype = Object.create(Error.prototype)

will prevent tree-shaking of DisposeAllError in most cases. Because assignments like this are treated as effectful, especially considering the fact that Object.create may not be whitelisted as "pure" in dead code eliminators like uglifyJS (it might be, aint sure about this, would have to check).

briancavalier · 2017-12-02T02:15:36Z

will prevent tree-shaking of DisposeAllError in most cases

Ah, ok, thanks for explaining. I'll add the IIFE.

briancavalier · 2017-12-02T02:24:11Z

Ok, there we go: 1019bda

Do you want me to cherry pick that onto this branch, and remove the babel plugin?

Andarist · 2017-12-02T10:40:14Z

Yeah 👍 that would simplify working with this branch :)

briancavalier · 2017-12-02T20:38:17Z

Done! Cherry-picked prototypal inheritance and removed builtin-extend plugin on this branch.

Andarist · 2017-12-02T21:12:22Z

Cool! This PR is ready to merge then, if you want to proceed with it. Let me know if it needs any other adjustments.

briancavalier · 2017-12-07T14:00:48Z

Here are results with webpack:

❯ /bin/ls -lrt counter/dist/
total 184
-rw-r--r--  1 brian  staff  25121 Dec  7 08:42 index.webpack.pure.js
-rw-r--r--  1 brian  staff   5481 Dec  7 08:44 index.webpack.pure.js.gz
-rw-r--r--  1 brian  staff  42553 Dec  7 08:55 index.webpack.base.js
-rw-r--r--  1 brian  staff   9486 Dec  7 08:56 index.webpack.base.js.gz

25121 / 42553 = ~59% of original non-gzip
5481 / 9486 = ~58% of original gzip

And here's the webpack config I used:

const path = require('path')
const webpack = require('webpack')
const UglifyJSPlugin = require('uglifyjs-webpack-plugin')

module.exports = {
  entry: './counter/src/index.js',
  output: {
    filename: 'index.js',
    path: path.resolve(__dirname, 'counter/dist')
  },
  resolve: {
    mainFields: [ 'module', 'jsnext:main', 'browser', 'main' ]
  },
  devtool: 'source-map',
  plugins: [
    new webpack.optimize.ModuleConcatenationPlugin(),
    new UglifyJSPlugin()
  ]
}

briancavalier · 2017-12-07T14:06:48Z

I pushed the two branches I used for the tests. Be sure to npm install in both the top level dir and the examples dir. You may also want to verify that for the PURE test, you have indeed ended up with PURE annotations in examples/node_modules/@most/core/dist/index.es.js. When I did the test the first time, somehow I missed a step and ended up without them--thus making it appear as if there was no savings at all.

test without PURE: https://github.com/mostjs/core/tree/x-webpack-tree-shake-master
test with PURE: https://github.com/mostjs/core/tree/x-webpack-tree-shake-pure

Andarist · 2017-12-07T15:08:54Z

Cool comparisons! Would be good to test webpack without ModuleConcatenationPlugin too, I predict though that it won't give even nearly similar savings - should save a little bit of something though. Good news is that webpack@4 is gonna have ModuleConcatenationPlugin enabled by default for production builds.

briancavalier · 2017-12-07T17:07:31Z

Without ModuleConcatenationPlugin (See index.webpack.pure-no-concat.*), surprisingly similar results. It may be because this is a trivial application, though.

❯ /bin/ls -lr counter/dist/
total 256
-rw-r--r--  1 brian  staff   5481 Dec  7 08:44 index.webpack.pure.js.gz
-rw-r--r--  1 brian  staff  25121 Dec  7 08:42 index.webpack.pure.js
-rw-r--r--  1 brian  staff   5655 Dec  7 12:05 index.webpack.pure-no-concat.js.gz
-rw-r--r--  1 brian  staff  26122 Dec  7 12:04 index.webpack.pure-no-concat.js
-rw-r--r--  1 brian  staff   9486 Dec  7 08:56 index.webpack.base.js.gz
-rw-r--r--  1 brian  staff  42553 Dec  7 08:55 index.webpack.base.js

webpack config:

const path = require('path')
// const webpack = require('webpack')
const UglifyJSPlugin = require('uglifyjs-webpack-plugin')

module.exports = {
  entry: './counter/src/index.js',
  output: {
    filename: 'index.js',
    path: path.resolve(__dirname, 'counter/dist')
  },
  resolve: {
    mainFields: [ 'module', 'jsnext:main', 'browser', 'main' ]
  },
  devtool: 'source-map',
  plugins: [
    // new webpack.optimize.ModuleConcatenationPlugin(),
    new UglifyJSPlugin()
  ]
}

briancavalier · 2017-12-07T17:41:16Z

@Andarist Huge thanks for starting the conversation in #138 and for this PR. @TrySound, thank you as well for the help and discussion.

I'm having a real "this is why open source is awesome" moment right now about the awesome collaboration 😄 ... and I'm excited that we'll be able to release such a concrete improvement for @most/core users!

I'm gonna go through the diff one last time to make sure we've tied up all loose ends.

Andarist · 2017-12-07T17:47:57Z

Actually results are similar because you are producing a flat bundle 👏

If you'd transpile file by file and produce a "mirror" directory structure results would be a lot worse probably and that would be exposed even more by inspecting a "trivial app" because in such a trivial app tree shaking should do more than for more complex app which would probably use a lot more of the core and thus benefits would be smaller.

I'm having a real "this is why open source is awesome" moment right now about the awesome collaboration 😄 ... and I'm excited that we'll be able to release such a concrete improvement for @most/core users!

Thanks for kind words :) I have just explored this area quite a bit lately and observed what prevents and what not tree shaking algos, so I've looked on several libs and tried to improve the situation where I could

davidchase

dynamite work 😍

Frikki

Having checked the builds, this LGTM.

briancavalier

🎉

briancavalier · 2017-12-07T23:25:27Z

Just published @most/core 1.1.0 with PURE annotations 🚀

Andarist commented Oct 4, 2017

View reviewed changes

Andarist force-pushed the babel branch from 2d82c82 to 8f5f765 Compare October 4, 2017 17:20

Andarist commented Oct 4, 2017

View reviewed changes

Andarist force-pushed the babel branch from 8f5f765 to eb07cf1 Compare October 4, 2017 17:21

Andarist commented Oct 4, 2017

View reviewed changes

Andarist force-pushed the babel branch from eb07cf1 to 396e768 Compare October 4, 2017 17:25

TrySound reviewed Oct 4, 2017

View reviewed changes

Andarist force-pushed the babel branch from 396e768 to ced9a8f Compare October 7, 2017 09:12

briancavalier mentioned this pull request Oct 9, 2017

chore(build): proof-of-concept PURE annotations #140

Closed

TylorS mentioned this pull request Oct 27, 2017

curry2 being called inside the index makes it not tree-shakeable TylorS/snabbdom-selector#25

Closed

briancavalier force-pushed the babel branch from ced9a8f to 6ab4da3 Compare November 30, 2017 12:15

briancavalier added 2 commits December 2, 2017 15:34

Use prototypal inheritance instead of extends for DisposeAllError

0023e3c

Remove now-unneeded babel builtin-extend plugin

bd7bc13

davidchase approved these changes Dec 7, 2017

View reviewed changes

Frikki self-requested a review December 7, 2017 19:39

Frikki approved these changes Dec 7, 2017

View reviewed changes

briancavalier approved these changes Dec 7, 2017

View reviewed changes

briancavalier merged commit 449ec9f into mostjs:master Dec 7, 2017

Frikki mentioned this pull request Dec 7, 2017

[discussion] Tree shake-ability #138

Closed

Andarist deleted the babel branch December 7, 2017 23:20

Babel MVP #143

Babel MVP #143

Conversation

Andarist commented Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist Oct 4, 2017 • edited Loading

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

Andarist Oct 4, 2017 • edited Loading

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

Andarist Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

TrySound commented Oct 4, 2017

Andarist commented Oct 4, 2017

briancavalier commented Oct 7, 2017

Andarist commented Oct 7, 2017

Andarist commented Oct 7, 2017

davidchase commented Oct 7, 2017

Andarist commented Oct 7, 2017

davidchase commented Oct 9, 2017

Andarist commented Oct 9, 2017

davidchase commented Oct 9, 2017

briancavalier commented Oct 11, 2017

briancavalier commented Oct 19, 2017

Andarist commented Oct 19, 2017

briancavalier commented Oct 19, 2017

Andarist commented Oct 21, 2017 • edited Loading

briancavalier commented Nov 28, 2017

Andarist commented Nov 28, 2017

briancavalier commented Nov 28, 2017

briancavalier commented Nov 28, 2017

briancavalier commented Nov 30, 2017

Andarist commented Nov 30, 2017

briancavalier commented Nov 30, 2017

Andarist commented Nov 30, 2017

briancavalier commented Nov 30, 2017

Andarist commented Nov 30, 2017

briancavalier commented Dec 1, 2017

Andarist commented Dec 1, 2017

briancavalier commented Dec 2, 2017

briancavalier commented Dec 2, 2017

Andarist commented Dec 2, 2017

briancavalier commented Dec 2, 2017

Andarist commented Dec 2, 2017

briancavalier commented Dec 7, 2017

briancavalier commented Dec 7, 2017

Andarist commented Dec 7, 2017

briancavalier commented Dec 7, 2017

briancavalier commented Dec 7, 2017

Andarist commented Dec 7, 2017

davidchase left a comment • edited Loading

Choose a reason for hiding this comment

Frikki left a comment

Choose a reason for hiding this comment

briancavalier left a comment

Choose a reason for hiding this comment

briancavalier commented Dec 7, 2017

Andarist commented Oct 4, 2017 •

edited

Loading

Andarist Oct 4, 2017 •

edited

Loading

Andarist Oct 4, 2017 •

edited

Loading

Andarist Oct 7, 2017 •

edited

Loading

Andarist commented Oct 21, 2017 •

edited

Loading

davidchase left a comment •

edited

Loading