Replace `browserify` with `rollup` #28

TrySound · 2020-10-12T17:44:22Z

Browserify: 13602 b
Rollup: 13585 b

Rollup is the best bundler to deal with es modules. In this diff I setup
it before migrating sources to esm. Achieved size is almost the same.

The config will be reused to produce cjs/esm entry points.

wooorm

Any idea where the couple of bytes reduction is coming from? Is it just because this version doesn‘t build as a standalone-UMD bundle?

package.json

wooorm · 2020-10-12T17:53:11Z

Looks like the bundle is output to stdout(4) on CI btw

TrySound · 2020-10-12T18:18:57Z

Actually I saw the same with browserify. Looks like something else prints code. Will check.

TrySound · 2020-10-12T18:19:38Z

The reduction comes from stripped debug I guess. It was not stripped with browserify in configuration.

wooorm · 2020-10-12T18:34:44Z

🤔 I didn't see that with browserify. It had a -o flag which wrote to a file I think?

wooorm · 2020-10-12T18:35:21Z

Debug is removed in the dist files already w babel, so that can't be it!

TrySound · 2020-10-12T19:40:46Z

lib was the input to browserify, not dist. So the size is grown a bit because of commonjs but we will achieve more optimisation after migrating to esm.

wooorm · 2020-10-12T19:45:30Z

Are you 100%? I checked that before. '.' Resolves to dest. You're saying this removes 20 bytes, but debug is much bigger than that so I have a hard time seeing it 🤔 tinyify is amazing, I can only understand the 20 byte shave coming from dropping the UMD bundle

TrySound · 2020-10-12T19:47:58Z

Ah, I see. You are right. I though main points to lib/index.js. Well, then I guess rollup just produces more optimal code then.

TrySound · 2020-10-12T19:50:19Z

Ah, and now it's in esm format, not cjs or umd.

TrySound · 2020-10-12T19:55:29Z

tinify is a set of optimisations around browserify + two passes of uglify. Btw I copy pasted it and passed to terser. Rollup does all optimisations out of the box.

TrySound · 2020-10-14T18:21:52Z

Here's per format sizes
Browserify: 13602 b
Rollup esm: 13585 b
Rollup cjs: 13594 b
Rollup umd: 13685 b

wooorm · 2020-10-20T08:55:44Z

Hi thanks for your patience! I wanted to take a break from this repo after having lived in here for several months 😅

Here's per format sizes
Browserify: 13602 b
Rollup esm: 13585 b [100b off from rollup umd, 17b off from browserify umd]
Rollup cjs: 13594 b [91b off from rollup umd, 8b off from browserify umd]
Rollup umd: 13685 b [83b bigger than browserify umd]

From these numbers, I conclude:
a) browerify does not output CJS/ESM or this isn’t checked.
b) rollup performs worse than browserify on comparable targets
c) the saving come from checking a different target: CJS or ESM over UMD.

So, what is the advantage of using rollup over browserify? The file from generate-size is not published: the numbers are used to check how big micromark would be in a browser. Switching to a different bundler/minifier will yields slightly different results, but that is unrelated to micromark.

TrySound · 2020-10-20T12:34:51Z

Afaik browserify cannot handle es modules without transpilation. Rollup supports only es modules out of the box. Migrating to esm will allow to scope hoist utilities, constants etc and make them mangleable. Browserify will probably not be able to show such reduction.

Ps: Im on vacation for the next two weeks. Cannot rebase.

wooorm · 2020-10-28T17:29:53Z

Afaik browserify cannot handle es modules without transpilation. Rollup supports only es modules out of the box.

 Both true I believe. 

Migrating to esm will allow to scope hoist utilities, constants etc and make them mangleable. Browserify will probably not be able to show such reduction. 

I strongly doubt that ESM improves these things. The code is already written with this in mind, and common-shakeify can do it too. ESM is an improvement over UMD—it can shave those 17 bytes off in this bundle we’re checking. But that is not a cost anyone actually has: if you bundle code, you’re not getting those bytes because you’re not getting an UMD bundle. In fact, adding ESM would add bytes for every file for every user: unifiedjs/rfcs#4 (comment)? I do see ESM as the way forward. I personally don’t see it as better in any way though. It’s just the blessed way.

If I am correct, and you’re interested in working on using ESM for micromark and compiling it with Rollup to CJS, then we’re getting another rollup config, correct? Would in then make sense to first do that while keeping browserify/tinyify (and maybe even keep on doing that in the future, or using both, so that we keep on comparing the same bundle that folks will typically get?)

Ps: Im on vacation for the next two weeks. Cannot rebase.

Have fun, take care, and please don‘t respond yet unless you want to!

TrySound · 2020-11-02T18:51:18Z

Gotcha. The problem was here

https://github.com/micromark/micromark/blob/main/lib/tokenize/block-quote.js#L50
https://github.com/micromark/micromark/blob/main/lib/tokenize/list.js#L155

Rollup wraps such dynamic usages with commonjs runtime which increased bundle size.

Now rollup umd bundle is 13443 (against 13566 with browserify)

Related-to GH-28. Closes GH-34. Reviewed-by: Marouane Fazouane <[email protected]> Reviewed-by: Titus Wormer <[email protected]>

Browserify: 13602 b Rollup: 13585 b Rollup is the best bundler to deal with es modules. In this diff I setup it before migrating sources to esm. Achieved size is the same. The config will be reused to produce cjs/esm entry points.

TrySound · 2020-11-18T13:53:21Z

Hi @wooorm Is it good to go?

wooorm

Hi! Thanks for your patience. Couple Qs and I’ll try this out locally now!

wooorm · 2020-11-18T15:49:12Z

package.json

    "generate": "npm run generate-constant-typings && npm run generate-expressions && npm run generate-dist && npm run generate-size",
-    "format": "remark . -qfo && prettier . -w --loglevel warn && xo --fix",
+    "format": "remark . -qfo && prettier --write . --loglevel warn && xo --fix",


Suggested change

"format": "remark . -qfo && prettier --write . --loglevel warn && xo --fix",

"format": "remark . -qfo && prettier . -w --loglevel warn && xo --fix",

This logs all output instead of writing.

lib/tokenize/block-quote.js

lib/tokenize/list.js

wooorm · 2020-11-18T15:52:50Z

rollup.config.js

+        mangle: {
+          safari10: true
+        }
+      })


Do you see differences for using terser twice? tinyify is running it on separate files, and on the whole, but this does the same twice. Maybe this isn’t noticable?

13425 vs 13589. But yeah, single pass is more realistic for user build.

ohhh, that’s significant! Weird! Well, let’s leave it in then

wooorm · 2020-11-18T15:59:48Z

Also: running npm test on the code here reformats a couple of comments/style things

package.json

TrySound · 2020-11-18T16:10:06Z

$ remark . -qfo && prettier . --write --loglevel warn && xo --fix
Error: Failed to load plugin 'prettier' declared in 'CLIOptions': Cannot find module 'eslint-plugin-prettier'

wooorm · 2020-11-18T16:13:25Z

rm -rf node_modules && npm i?

TrySound · 2020-11-18T16:15:57Z

It works. Thanks. Btw lockfile allows to avoid this.

wooorm · 2020-11-18T16:19:02Z

lockfiles

I have only heard of this error for folks using yarn. Can you confirm whether that was the case?

Comparing your branch to main, I’m now getting:

rollup:
13.4 kB
13436

browserify / tinyify:
13.5 kB
13479

Rollup/terser removes 43 bytes! Nice, but pretty small. Again: ESM is the future, but that is soooo small. ESM isn’t better than CJS at tree shaking 🙄

TrySound · 2020-11-18T16:30:18Z

I have only heard of this error for folks using yarn. Can you confirm whether that was the case?

Ah, actually you are right. I'm tied to yarn lockfiles 😄

Rollup/terser removes 43 bytes! Nice, but pretty small.

It's 13386 with simplified blockquote and list exports. And remember, the code is still commonjs. Rollup may do less assumptions about safe commonjs transformations. Maybe it's also new commonjs plugin version.

wooorm · 2020-11-18T16:37:32Z

Yeah, you’re right! Maybe that’ll help!

wooorm · 2020-11-18T16:38:50Z

package.json

    "generate": "npm run generate-constant-typings && npm run generate-expressions && npm run generate-dist && npm run generate-size",
-    "format": "remark . -qfo && prettier . -w --loglevel warn && xo --fix",
+    "format": "remark . -qfo && prettier . --write --loglevel warn && xo --fix",


Suggested change

"format": "remark . -qfo && prettier . --write --loglevel warn && xo --fix",

"format": "remark . -qfo && prettier . -w --loglevel warn && xo --fix",

haha, you really like the long form 😅 If you feel strongly, I’m fine with it. Otherwise, you can apply this suggestion

Ah, short form works with reinstalled node_modules. Probably was a bug in old prettier. But yeah, let's use long version for consistency :)

Uhm, the consistency is that short flags are used where possible, and it was that way before, hence why I’m suggesting to go back to how it was 😅

Anyway, feel free to commit this suggestion, or not. I think the rest looks good. Is this PR ready to go? Anything else?

In other script --write was used

wooorm · 2020-11-18T16:39:14Z

rollup.config.js

+        mangle: {
+          safari10: true
+        }
+      })


ohhh, that’s significant! Weird! Well, let’s leave it in then

wooorm · 2020-11-19T10:08:24Z

Awesome, thanks @TrySound!

* Add tests for local babel plugins * Replace babel w/ rollup * Remove unused constant Related-to unifiedjs/rfcs#4. Related-to GH-27. Related-to GH-28. Closes GH-35. Reviewed-by: Titus Wormer <[email protected]>

wooorm reviewed Oct 12, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

wooorm added 🏗 area/tools This affects tooling 🙉 open/needs-info This needs some more info labels Oct 20, 2020

TrySound force-pushed the rollup branch from b1cc0cd to d3a8608 Compare November 2, 2020 18:52

wooorm pushed a commit that referenced this pull request Nov 11, 2020

Add skipLibCheck to ignore unrelated type tests

c78c1c2

Related-to GH-28. Closes GH-34. Reviewed-by: Marouane Fazouane <[email protected]> Reviewed-by: Titus Wormer <[email protected]>

wooorm mentioned this pull request Nov 11, 2020

types: skip lib check #34

Merged

TrySound added 6 commits November 11, 2020 11:36

Replace browserify with rollup

11e28d7

Browserify: 13602 b Rollup: 13585 b Rollup is the best bundler to deal with es modules. In this diff I setup it before migrating sources to esm. Achieved size is the same. The config will be reused to produce cjs/esm entry points.

Down rollup versions and fix prettier command

c4864e7

Fix lint

a59e5d3

use const binding

d3abdb7

Fix commonjs dynamism and bundle umd instead

2b88d3c

Upgrade rollup plugins

888b1dc

TrySound force-pushed the rollup branch from 915ff4c to 888b1dc Compare November 11, 2020 08:37

This comment has been minimized.

Sign in to view

wooorm reviewed Nov 18, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

wooorm reviewed Nov 18, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

Put target first in prettier

f919513

TrySound added 2 commits November 18, 2020 19:13

Enable rollup silent flag

63e7fb2

Format code

81ad54b

Make block-quote and list default exports

f2541f8

wooorm reviewed Nov 18, 2020

View reviewed changes

Use write alias for both scripts with prettier

e76d9a1

wooorm approved these changes Nov 19, 2020

View reviewed changes

wooorm changed the title ~~Replace browserify with rollup~~ Replace browserify with rollup Nov 19, 2020

wooorm merged commit 91745b4 into micromark:main Nov 19, 2020

wooorm added ⛵️ status/released 🦋 type/enhancement This is great to have and removed 🙉 open/needs-info This needs some more info labels Nov 19, 2020

ChristianMurphy mentioned this pull request Dec 22, 2020

Build Modernization mdx-js/mdx#1271

Closed

wooorm added the 💪 phase/solved Post is done label Apr 12, 2021

	"format": "remark . -qfo && prettier --write . --loglevel warn && xo --fix",
	"format": "remark . -qfo && prettier . -w --loglevel warn && xo --fix",

Replace browserify with rollup #28

Replace browserify with rollup #28

Conversation

TrySound commented Oct 12, 2020 • edited Loading

wooorm left a comment

Choose a reason for hiding this comment

wooorm commented Oct 12, 2020

TrySound commented Oct 12, 2020

TrySound commented Oct 12, 2020

wooorm commented Oct 12, 2020

wooorm commented Oct 12, 2020

TrySound commented Oct 12, 2020

wooorm commented Oct 12, 2020

TrySound commented Oct 12, 2020

TrySound commented Oct 12, 2020

TrySound commented Oct 12, 2020

TrySound commented Oct 14, 2020

wooorm commented Oct 20, 2020

TrySound commented Oct 20, 2020

wooorm commented Oct 28, 2020

TrySound commented Nov 2, 2020 • edited Loading

This comment has been minimized.

TrySound commented Nov 18, 2020

wooorm 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

wooorm commented Nov 18, 2020

TrySound commented Nov 18, 2020

wooorm commented Nov 18, 2020

TrySound commented Nov 18, 2020

wooorm commented Nov 18, 2020

TrySound commented Nov 18, 2020 • edited Loading

wooorm commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooorm Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooorm commented Nov 19, 2020

Replace `browserify` with `rollup` #28

Replace `browserify` with `rollup` #28

TrySound commented Oct 12, 2020 •

edited

Loading

TrySound commented Nov 2, 2020 •

edited

Loading

TrySound commented Nov 18, 2020 •

edited

Loading

wooorm Nov 19, 2020 •

edited

Loading