[WIP] build: generate commonjs, es module, and umd versions of unified #76

ChristianMurphy · 2019-10-31T23:40:53Z

Goal

Generate several distribution versions for different environments, to allow minimizing downstream tooling to minimize the download/bundle/build size.

Approach

This leverages microbundle, to generate a common js build for Node, a UMD build for all browsers, and an ES Module build for modern browsers.
Microbundle also minifies the builds and includes a sourcemap for simpler debugging of minified build.

Alternatives

Use babel, rollup, webpack, or other build tools to generate distribution versions
Continue to distribute only a CJS version
Hand code both a CJS and a ESM version
Migrate to typescript, use tsc to generate multiple outputs

ChristianMurphy · 2019-10-31T23:45:16Z

These changes were inspired by work on https://github.com/syntax-tree/mdast-util-from-quill-delta
which is intended for use in browser environments.

A few tweaks to the build to generate ESM, mark the package as side effect free, and drop some polyfills that can be applied by the build process cut the package size in half from v0.1.0 to v0.1.3.
https://bundlephobia.com/[email protected]

Whether or not this PR is merged, would be interested in taking some of the ideas/optimizations from there and applying them to unified itself.

ChristianMurphy · 2019-10-31T23:46:58Z

package.json

    "test-api": "node test",
-    "test-coverage": "nyc --reporter lcov tape test",
+    "test-coverage": "c8 --reporter=lcov --check-coverage --lines 100 --functions 100 --branches 100 tape test",


also migrate coverage to c8, which leverages V8's native coverage capabilities, since the builds now are done on Node 10+
https://github.com/bcoe/c8

May open a separate PR just for this.

C8 seems to be pretty young, unstable (see pipelineParse), and include bugs. What’s the benefit of switching?

(oh and see: #76 (comment))

C8 seems to be pretty young, unstable

Young-ish, 1.0 was released in Oct 25, 2017, over two years ago.
It is as stable as the coverage feature in v8.

and includes bugs

🤷‍♂️ so do Nyc and Istanbul

https://github.com/istanbuljs/nyc/issues

https://github.com/istanbuljs/istanbuljs/issues

What’s the benefit of switching?

It is a native feature of Chrome/Node and should more accurately reflect runtime coverage, than the build-instrumentation based approach of Instanbul (some work may still be needed), as well has having the performance boost from running directly in the platform.

ChristianMurphy · 2019-10-31T23:49:05Z

package.json

+  "source": "index.js",
+  "main": "dist/unified.js",
+  "module": "dist/unified.mjs",
+  "unpkg": "dist/unified.umd.js",
  "types": "types/index.d.ts",


These are the different build types recommended by microbundle
https://github.com/developit/microbundle#set-up-your-packagejson

The recommendations also line up pretty well with Babel's recommendations
https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages

ChristianMurphy · 2019-10-31T23:49:51Z

package.json

  "types": "types/index.d.ts",
+  "sideEffects": false,


this flag is specifically for bundlers, it signals that additional tree-shaking optimizations can be safely made.
https://webpack.js.org/guides/tree-shaking/

ChristianMurphy · 2019-10-31T23:50:31Z

index.js

@@ -461,3 +457,6 @@ function assertDone(name, asyncName, complete) {
    )
  }
 }
+
+// Expose a frozen processor.
+export default unified().freeze()


This was moved to the end due to bcoe/c8#66

wooorm

This is interesting and we should do this somewhere in the future, but now?

I can’t run this code without compiling it. This would probably be a major release and something that bubbles through 250 projects like TS definitions. ESM in Node is still unstable, but should be there soon, so I’m thinking it’ll be solid in Node 14, which I propose we wait for?

P.S. I think it’s different for apps: you can do whatever, but for libraries I prefer less frequent changes and not using beta features.

wooorm · 2019-11-01T08:14:50Z

index.js

+      var pipeline = trough()
+        .use(pipelineParse)
+        .use(pipelineRun)
+        .use(pipelineStringify)


Wouldn’t this make every run significantly slower?

Maybe, benchmarking would give better insights on the potential impact.

wooorm · 2019-11-01T08:22:13Z

index.js

+import bail from 'bail'
+import vfile from 'vfile'
+import trough from 'trough'
+import plain from 'is-plain-obj'

 function pipelineParse(p, ctx) {


For some reason coverage doesn’t see this function as covered?

This is likely also related to bcoe/c8#66

wooorm · 2019-11-01T08:24:39Z

package.json

    "test-api": "node test",
-    "test-coverage": "nyc --reporter lcov tape test",
+    "test-coverage": "c8 --reporter=lcov --check-coverage --lines 100 --functions 100 --branches 100 tape test",


C8 seems to be pretty young, unstable (see pipelineParse), and include bugs. What’s the benefit of switching?

ChristianMurphy · 2019-11-01T13:09:53Z

This is interesting and we should do this somewhere in the future, but now?

I can’t run this code without compiling it. This would probably be a major release and something that bubbles through 250 projects like TS definitions. ESM in Node is still unstable, but should be there soon, so I’m thinking it’ll be solid in Node 14, which I propose we wait for?

P.S. I think it’s different for apps: you can do whatever, but for libraries I prefer less frequent changes and not using beta features.

For Node, Modules will likely be marked as stable in the next minor release of 13.
https://twitter.com/MylesBorins/status/1189618753065144322

For browsers and bundlers: modules have been supported for a while.

wooorm · 2019-11-01T15:47:33Z

While browsers are important, and they do support ESM for a bit now, I would argue that currently ESM is written practically only for compilers, and only experimentally for browsers. People write for Webpack/Rollup, a “fake” ESM, not for the actual HTTP/2 benefits of ESM.

From the Twitter thread, and clicking through on comments, reading GH issues, and watching the conversation, it seems there are still things undecided, or at least differences between this PR here (e.g, type: 'module').
It seems that Node’s first priority is to ship ESM in Node without a flag, and then there is a January deadline for native conditional exports (supporting both CJS or MJS). I also see that they want to backport to Node@12, which may take 3-6 months.

My reason for wanting to wait for 12 and 14, is that those are LTSs, that most users will be on then. At that time, ESM in Node will have figured out all the inevitable bugs that will show up for such a years long process.

I don’t think it’s responsible to move a whole ecosystem/millions of users over now, in such a transitioning phase

wooorm · 2019-11-01T17:04:06Z

You mentioned that quill was halved in size, do you have the numbers of this change for unified?

ChristianMurphy · 2019-11-01T17:47:36Z

My reason for wanting to wait for 12 and 14, is that those are LTSs, that most users will be on then. At that time, ESM in Node will have figured out all the inevitable bugs that will show up for such a years long process.

I don’t think it’s responsible to move a whole ecosystem/millions of users over now, in such a transitioning phase

There may be some confusion here.
To clarify, this update would still ship a common js, Node 8/10+ compatible build, as the default main.
Adopters using node will continue to get the same code after this, as they are today.

The difference is adding more module options.
Which bundlers, modern browsers, and Node 13 have the option of using.

ChristianMurphy · 2019-11-01T20:54:26Z

You mentioned that quill was halved in size, do you have the numbers of this change for unified?

Comparing the minified file produced by this build to the one from the previous release.
The the release index file is 40.3 KB and the release minified file (with bundled dependnencies) is 12.5 KB
The minified file from this build is 3.4 KB (not including dependencies)

The difference at this level isn't massive, just 7-9 KB.
My thinking is, if this approach works out, and can make it's way through the unified ecosystem.
The impact would be orders of magnitude larger when adopting remark + rehype + plugins or mdx.
If many/most of the modules support tree shaking.

wooorm · 2019-11-02T13:50:33Z

Your statement “The difference at this level isn't massive, just 7-9 KB” seems to imply that with ESM and microbundle, 7-9 KB is saved, but that is not the case: that number comes from dependencies that are included in the current bundle, and will have to be included in any bundle.

The current bundle is 3.3kb minzipped, of which 37.2% is from unified itself (excluding dependencies), so unified without dependencies is currently 1.2276 kB minzipped.
I verified that this number is roughly correct using browserify index.js -p tinyify --no-bundle-external | gzip-size, which gives 1.33 kB (and 1.42 kB with -s unified for a UMD bundle).

With this PR, I get 1.38 kB for dist/unified.mjs, 1.41 kB for dist/unified.js, and 1.53 kB for dist/unified.umd.js. The results are similar to the status quo, so ESM is not an improvement size-wise. This is all of course a discussion based on a fictional problem, as people normally don’t bundle unified without using it or combining it with plugins.

My thinking is, if this approach works out, and can make it's way through the unified ecosystem.
The impact would be orders of magnitude larger when adopting remark + rehype + plugins or mdx.
If many/most of the modules support tree shaking.

I refute that, because a) I found that there is no real size difference, as shown above, b) tree shaking can be done on CJS pretty well too, and c) tree shaking is useful for monoliths, where branches can be ignored, whereas unified is modular, and packages act as contained branches

ChristianMurphy · 2019-11-02T14:37:03Z

tree shaking can be done on CJS pretty well too

This assumes users can/will use browserify.
npm download stats suggest otherwise, in the past week:

webpack had 8.4M downloads
rollup had 0.9M downloads
browserify 0.6M downloads
parcel had 0.1M downloads

webpack only tree shakes esm dependencies https://webpack.js.org/guides/tree-shaking

we could add the additional alternative, to document browserify, rollup, and parcel to steer users in that direction (as part of unifiedjs/unifiedjs.github.io#7), and/or to document that webpack users should include https://github.com/indutny/webpack-common-shake

tree shaking is useful for monoliths, where branches can be ignored, whereas unified is modular, and packages act as contained branches

counter example, mdx.
mdx relies on babel for JSX parsing, babel is a majority of @mdx/runtime's bundle size
https://bundlephobia.com/result?p=@mdx-js/[email protected]
tree shaking would have opportunities to reduce bundle size, despite mdx's modular nature.
There are likely more examples like this throughout the unified ecosystem.

What this is still overlooking in the potential savings of a bundler-less setup, like pika pack.
https://www.pika.dev/blog/pika-web-a-future-without-webpack

Where supporting ESM would allow for HTTP2 optimizations and keeping partial caches even when dependencies change.

wooorm · 2019-11-02T15:04:58Z

This assumes users can/will use browserify.

I meant to imply that it can be done, not that it can only be done with browserify. As you show, common-shake can be used in webpack as well.
I think it is unfortunate that projects like rollup and others misguide people into thinking that ESM “supports” tree shaking and CJS doesn’t. With both, you need a compiler to do that, and both formats support it.

guides

Yes, good idea! 👍

mdx-js/runtime

Babel/MDX already have ESM! And the runtime is not really meant to be used: it’s super unsafe, and the size is documented.

There are likely more examples like this throughout the unified ecosystem.

I don’t think there are though?

Where supporting ESM would allow for HTTP2 optimizations and keeping partial caches even when dependencies change

Yeah that’s very interesting!
I’m not fighting against ESM. I am want to delay using it until Node has ESM and it is more stable.

wooorm · 2019-11-02T15:45:00Z

Does the side effects field have affect for CJS?
Despite the few problems in C8, I think we could still switch to that?

ChristianMurphy · 2019-11-02T18:05:53Z

Does the side effects field have affect for CJS?

For webpack: no
For rollup: yes
For browserify: no
For parcel: maybe?

Despite the few problems in C8, I think we could still switch to that?

Can open a PR just for that to discuss further.

wooorm · 2019-11-03T16:12:56Z

Where did you find that rollup supports CJS sideEffects? With a quick Google I couldn’t find that. Anyway, if it does, I think we should add it!

Can open a PR just for that to discuss further.

I think C8 would be useful, bcoe does good work so it’s probably worth it to switch to the successor of nyc!

ChristianMurphy · 2019-11-03T16:33:57Z

Where did you find that rollup supports CJS sideEffects? With a quick Google I couldn’t find that. Anyway, if it does, I think we should add it!

From reading between the lines a bit.

To tree shake rollup converts CJS to ESM https://github.com/rollup/rollup-plugin-commonjs
Then these issues seem to suggest that the tree shaking pipeline is the same for both from there forward: rollup/rollup-plugin-commonjs#362, rollup/rollup#898, and rollup/rollup#902

build: generate commonjs, es module, and umd versions of unified

cf69ebf

ChristianMurphy requested a review from a team October 31, 2019 23:40

ChristianMurphy commented Oct 31, 2019

View reviewed changes

wooorm reviewed Nov 1, 2019

View reviewed changes

ChristianMurphy closed this Nov 2, 2019

ChristianMurphy deleted the build/generate-umd-cjs-and-esm branch November 2, 2019 15:16

ChristianMurphy added 🙅 no/wontfix This is not (enough of) an issue for this project and removed 🛠 blocked/wip This cannot progress yet, it’s being worked on 🙉 open/needs-info This needs some more info labels Nov 2, 2019

ChristianMurphy mentioned this pull request Nov 3, 2019

Refactor coverage tests to use c8 instead of nyc #77

Merged

ChristianMurphy mentioned this pull request Apr 7, 2020

Adopt ES2015+ features in Unified collective unifiedjs/rfcs#4

Merged

ChristianMurphy mentioned this pull request Sep 11, 2020

types: add type definitions for micromark micromark/micromark#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] build: generate commonjs, es module, and umd versions of unified #76

[WIP] build: generate commonjs, es module, and umd versions of unified #76

ChristianMurphy commented Oct 31, 2019

ChristianMurphy commented Oct 31, 2019

ChristianMurphy Oct 31, 2019

wooorm Nov 1, 2019

wooorm Nov 1, 2019

ChristianMurphy Nov 1, 2019

ChristianMurphy Oct 31, 2019

ChristianMurphy Oct 31, 2019

ChristianMurphy Oct 31, 2019

wooorm left a comment

wooorm Nov 1, 2019

ChristianMurphy Nov 1, 2019

wooorm Nov 1, 2019

ChristianMurphy Nov 1, 2019

wooorm Nov 1, 2019

ChristianMurphy commented Nov 1, 2019

wooorm commented Nov 1, 2019

wooorm commented Nov 1, 2019

ChristianMurphy commented Nov 1, 2019 •

edited

Loading

ChristianMurphy commented Nov 1, 2019 •

edited by wooorm

Loading

wooorm commented Nov 2, 2019

ChristianMurphy commented Nov 2, 2019

wooorm commented Nov 2, 2019

wooorm commented Nov 2, 2019

ChristianMurphy commented Nov 2, 2019

wooorm commented Nov 3, 2019

ChristianMurphy commented Nov 3, 2019

[WIP] build: generate commonjs, es module, and umd versions of unified #76

[WIP] build: generate commonjs, es module, and umd versions of unified #76

Conversation

ChristianMurphy commented Oct 31, 2019

Goal

Approach

Alternatives

ChristianMurphy commented Oct 31, 2019

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

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

ChristianMurphy commented Nov 1, 2019

wooorm commented Nov 1, 2019

wooorm commented Nov 1, 2019

ChristianMurphy commented Nov 1, 2019 • edited Loading

ChristianMurphy commented Nov 1, 2019 • edited by wooorm Loading

wooorm commented Nov 2, 2019

ChristianMurphy commented Nov 2, 2019

wooorm commented Nov 2, 2019

wooorm commented Nov 2, 2019

ChristianMurphy commented Nov 2, 2019

wooorm commented Nov 3, 2019

ChristianMurphy commented Nov 3, 2019

ChristianMurphy commented Nov 1, 2019 •

edited

Loading

ChristianMurphy commented Nov 1, 2019 •

edited by wooorm

Loading