Replace bublé with babel #263

eliihen · 2018-12-04T20:30:54Z

Hello 👋

First of all - thank you so much for this project, it makes following best practices when packaging libraries for npm really easy!

I saw the PR at #39 and thought I should help out by merging in the latest code from master and getting it up to date. However, a lot seems to have changed since #39 was created, so it was actually a lot easier to start from scratch. So that's what I did, and in this branch it now works with babel 7.

Reading the discussion in #39, I interpreted @developit's comment as intent to replace bublé with babel, so that's what this PR does.

as long as we can get a default babel config that is as optimized as Bublé, we can just default to Babel

The generated configuration uses a default babel config consisting of preset-env and preset-react, alongside the existing plugins for async/await support. If a babel config file is detected, it uses a clean slate. I hope this is a sensible starting point; free to suggest changes or improvements.

I kept the custom shebang plugin even though it references bublé because the build fails without it.

The tests are currently failing due to increased output sizes. I wanted to get some feedback before I update the snapshots. Should we look into reducing them before going forward with this?

A lot of inspiration in this PR was taken from @Andarist in #39. Thanks a lot for the pointers on where to look! Hope it's okay with you that I did it this way.

Fixes #25

ForsakenHarmony · 2018-12-05T00:57:04Z

Yes, size is important that's why we currently only use babel for async await

package.json

Andarist · 2018-12-05T08:22:34Z

src/index.js

-						],
-					}),
+					babel(
+						(await isFile('.babelrc')) || (await isFile('.babelrc.js'))


seems like it should get hidden in the separate function as the logic here is complex-ish and its hard to read when its inlined

also this is not quite the only way to configure babel - there is also babel.config.js and babel key in the package.json

I think there is some API in babel@7 to detect if there is any configuration for the project. Maybe you could search for it to see if it meets our needs, you could also try to research what storybooks and babel-jest are doing atm (although both may have custom implementations and it would better to use babel-provided one if possible)

after a thought - we should make our internal babel config a public babel-preset-microbundle, but should be done in a followup review - because it will require setting up a monorepo.

ohh I like that idea of a dedicated microbundle preset 👍

@Andarist Could we expose something like https://github.com/babel/babel-loader/#customized-loader from rollup-plugin-babel? I think it is very bad to encourage frameworks to implement their own resolution for .babelrc files and such, since it doesn't necessarily conform to Babel's own implementation details.

@Andarist Could we expose something like https://github.com/babel/babel-loader/#customized-loader from rollup-plugin-babel?

Sure, gonna look through this to see what it actually is & how it can be used to achieve goals here.

I think it is very bad to encourage frameworks to implement their own resolution for .babelrc files and such, since it doesn't necessarily conform to Babel's own implementation details.

Totally agreed - I was only pointing to babel-jest & others to see how similar goal is achieved. It's 100% better to reuse the logic in babel itself to avoid buggy implementations.

src/index.js

Andarist · 2018-12-05T08:33:45Z

Could u paste in formatted output of a single failed snapshot pre & post this PR? We could see where lies the difference in transpiling and decide how to proceed further with this.

Thanks for your work!

eliihen · 2018-12-05T09:26:55Z

Thanks for the excellent review @Andarist! 👏 I'll update the PR later today and we can discuss how to proceed further.

eliihen

The PR has now been updated with the requested changes.

I found a function in @babel/core called loadPartialConfig that enables validation of configuration files. This is a documented API and is pretty much what we are looking for. Please note the discussion I started in the new diff, however.

The snapshot diffs have now been pushed for your commenting pleasure. After perusing them one more time, it seems not all the diffs are up, some are in fact down. So it's a lot more of a mixed bag than I originally thought. We may still want to look into reducing the file size, though, I'm open for suggestions here.

eliihen · 2018-12-05T19:24:45Z

src/index.js

@@ -226,6 +226,45 @@ export default async function microbundle(options) {
 	);
 }

+function createBabelConfig(options) {
+	const partialConfig = loadPartialConfig();
+	const hasBabelConfigFile = partialConfig.hasFilesystemConfig();


Note that this function does not respect the babel field in package.json. The only way I can see is adding a special case for detecting this field. Thoughts?

The thing is that loadPartialConfig is meant to be called per-file, because some files may have a .babelrc and some may not. That is what drove my comment above. It seems like rollup-plugin-babel should be exposing this as a feature, which is what we do for babel-loader so that people can do the same thing.

@loganfsmyth: I see, thanks for the clarification. So if we go for that solution, that would mean this is impeded on rollup-plugin-babel exposing this feature.

I can make an issue over at rollup-plugin-babel to the best of my ability and see where that takes us.

Andarist · 2018-12-05T20:27:21Z

As to the snapshots - it would be great to see comparisons of the output code, not the size differences.

The PR looks really good, we "only" (from my perspective, other collaborators have to state their minds too about this) need to investigate size differences and implement config loading related feature in rollup-plugin-babel (im a maintainer, so we should be able to move with it quickly if u want to work on it or I can research the topic in a few days and implement it myself)

eliihen · 2018-12-06T12:21:01Z

Here's the list of changes to the compiled output from the tests before and after this change. It's formatted with prettier to make it easier to read, so the linked gists will not match 1-1 with the bytes listed in the table below.

The byte sizes below are the file sizes of the js bundle before compression.

Filename	Before	After	Change	Before	After
async-iife-ts	116	116	0%	before	after
async-ts	253	120	-52.5%	before	after
basic-css.js	133	133	0%	before	after
basic-json.js	168	113	-32.7%	before	after
basic-lib.js	468	502	7.3%	before	after
basic-lib-ts.js	130	147	13%	before	after
basic-lib-tsx.js	304	344	13.16%	before	after
class-decorators-ts.js	591	591	0%	before	after
class-properties.js	127	127	0%	before	after
custom-source.js	472	506	7.2%	before	after
esnext-ts.js	2000	2490	24.5%	before	after
jsx.js	294	252	-14.3%	before	after
name-custom-amd.js	474	508	7.2%	before	after
name-custom.js	470	504	7.2%	before	after
no-pkg.js	465	499	7.3%	before	after
no-pkg-name.js	470	504	7.2%	before	after
pretty.js	12522	12522	0%	before	after
raw.js	2960	2960	0%	before	after
shebang.js	101	101	0%	before	after

Andarist · 2018-12-06T14:35:43Z

test case	reason of the diff
basic-json	babel has removed iterating through arguments because `args` stayed unreferenced, this for sure is OK in loose mode which we want to use, probably even OK in general case (cant find in my head a thing that it could break even in very contrived situations)
basic-lib	when spreading arguments babel is initializing created array with `new Array(length)`, buble creates `[]`. Allocating upfront is probably a little bit better here, so I would classify it as a good tradeoff. Also - babel iterates from left to right, buble iterated from right to left and leverages the fact that the end condition will be met automatically when using i-- and reaching 0, we could propose this to babel as a minor output optimization.
basic-lib-ts	it has untranspiled class, so this is a bug somewhere
basic-lib-ts	again untranspiled stuff - classes, arguments rest & arrow function, seems like TS transpilation is broken
class-decorators-ts	again untranspiled TS stuff
custom-source	same as basic-lib
esnext-ts	untranspiled `const` declaration which prevents being able to merge it with other variable declarations there
jsx	same reason as basic-json
name-custom-amd	same as basic-lib
name-custom	same as basic-lib
name-pkg	same as basic-lib
name-pkg-name	same as basic-lib

eliihen · 2018-12-06T17:38:07Z

again untranspiled stuff - classes, arguments rest & arrow function, seems like TS transpilation is broken

Yes, compilation of TypeScript files was missing from the rollup-plugin-babel extension set. Fixed that now and updated the gist and the output comment with updated file sizes.

TS is now up in size, except for decorators, which is now flat.

One annoyance that popped up now is the failing test, see below. This seems to be the same issue as rpetrich/babel-plugin-transform-async-to-promises#20

 fixtures › esnext-ts
    ReferenceError: Container is falsy
      at NodePath._replaceWith (node_modules/@babel/traverse/lib/path/replacement.js:188:11)
      at NodePath._remove (node_modules/@babel/traverse/lib/path/removal.js:51:10)
      at NodePath.remove (node_modules/@babel/traverse/lib/path/removal.js:30:8)
      at Object.VariableDeclaration (node_modules/babel-plugin-transform-async-to-promises/async-to-promises.ts:1010:21)
      at NodePath._call (node_modules/@babel/traverse/lib/path/context.js:53:20)
      at NodePath.call (node_modules/@babel/traverse/lib/path/context.js:40:17)
      at NodePath.visit (node_modules/@babel/traverse/lib/path/context.js:88:12)
      at TraversalContext.visitQueue (node_modules/@babel/traverse/lib/context.js:118:16)
      at TraversalContext.visitSingle (node_modules/@babel/traverse/lib/context.js:90:19)
      at TraversalContext.visit (node_modules/@babel/traverse/lib/context.js:146:19)

src/index.js

Andarist · 2018-12-06T20:22:18Z

Additional note - babel wraps transpiled class in an IIFE which increases the size a little bit, BUT by adding #PURE comments to those IIFEs it makes them treeshakable (so its superior in this sense to buble). Unfortunately static assignments, such as assignments to prototypes, deopts most treeshaking/DCE algorithms so its better to avoid those in top level scope.

src/index.js

developit · 2018-12-08T00:53:39Z

A bunch of them fall into the 7.2% increase that I think is worth figuring out. I don't think the Array preallocation is actually beneficial in the case of arguments - Buble was right in doing a backwards loop, since that results in the Array being initialized and immediately set to the correct length (largest index first).

Perhaps we can get Babel to change its output format for rest parameters?

developit · 2018-12-08T01:46:13Z

I've opened a PR to Babel to make the outputs align: babel/babel#9152

eliihen · 2018-12-10T20:24:18Z

According to the latest comment in rpetrich/babel-plugin-transform-async-to-promises#20, the issue with the failing test should be fixed. I just tried running the esnext-ts.js test after taking in the latest changes from babel-plugin-transform-async-to-promises master, and sure enough, it does run. The results, however, are seemingly horrible. The output has gone from 891 to 2779 bytes. That's an increase of over 211.9%!

I scanned through the diffs, and I see the "before" (bublé) has async/await in the output! The "after" (babel) does not. This obviously results in the babel output being much, much larger.

It seems async/await with typescript is bugged on HEAD of master (210bb00), at least in this test. Maybe related to #259?

I'll push the new output to the main gist once babel-plugin-transform-async-to-promises releases a new version. For now you can find it here: https://gist.github.com/esphen/61e5b37e20573b9508713f888e22c227. Compare with bublé here

rpetrich · 2018-12-10T20:50:49Z

@esphen A good portion of that output is tslib's __asyncValues helper which is used to transform for await when TypeScript is configured to target es2017; switching to target esnext will use babel-plugin-transform-async-to-promises's equivalent helper which is more compact. Also, I plan to publish later this evening.

eliihen · 2018-12-11T07:31:02Z

@rpetrich Thanks for the tip. I tried setting typescript's target to esnext, and that resulted in reducing the bundle size from 2779 to 2490 bytes (-10.4%). Should also mention that the only test that changed in size was esnext-ts.

I would like to hear from the other maintainers before we hand over responsibility of compiling ES from the ts plugin to babel, however. Currently typescript only compiles to ES2017, then bublé/babel does the rest. Any good reason it is done this way currently?

Andarist · 2018-12-11T10:57:41Z

Could you rebase on top of the master?

I would like to hear from the other maintainers before we hand over responsibility of compiling ES from the ts plugin to babel, however. Currently typescript only compiles to ES2017, then bublé/babel does the rest. Any good reason it is done this way currently?

I guess the reasoning is to strip types with TS and do the actual transpilation to ES5 with buble/babel

FezVrasta · 2018-12-11T12:47:50Z

src/index.js

+	}
+
+	// No babelrc exists, use default config
+	// TODO move this into a separate babel-preset-microbundle package


Could we start by moving this into its own file?

FezVrasta · 2018-12-11T12:49:07Z

src/index.js

-							],
-						],
-					}),
+					babel(createBabelConfig(options)),
 					{
 						// Custom plugin that removes shebang from code because newer


Do we still need this? If yes maybe it's worth to update the comment to explain why

FezVrasta · 2018-12-11T13:01:09Z

Could you rebase on top of the master?

I sent a PR to @esphen's branch to rebase to master (eliihen#1)

rebase to master

eliihen · 2018-12-11T20:11:36Z

New test results are in.

I've merged @FezVrasta's changes and updated the comment on the output sizes above. Notable changes:

Added async-ts and async-iife-ts tests from master.
- async-ts is down because babel has successfully tree-shaken the unused async function
- async-iife-ts is flat
Fixed the esnext-ts sizes, both before and after
- esnext-ts is now up by 24.5%
- Have not quite figured out why yet, feel free to dig in

Andarist · 2018-12-12T12:04:54Z

@esphen have you updated the comparison table with new versions of those outputs? I might compare them later.

eliihen · 2018-12-12T12:11:44Z

@Andarist Yes, both the comparison table and the gist are up to date

# Conflicts: # package.json # src/index.js # test/__snapshots__/index.test.js.snap

wardpeet · 2019-03-12T20:45:06Z

@ForsakenHarmony @developit do we want this getting merged? I can put my teeth in it as we want this for gatsbyjs too so we can add our own babelrc file or will we still keep babelrc false?

developit · 2019-03-19T14:12:55Z

@wardpeet yeah. We definitely still want to move off Bublé.