Dual-build package as ES modules and CommonJS #200

robertknight · 2021-09-23T08:17:11Z

This will smooth the migration of downstream applications to a new bundler that speaks ES modules natively [1]. On this branch the existing lib/ dir and .babelrc module now relate to the ES module build, and the lib-cjs/
dir and .babelrc-cjs module are for the CommonJS build. Once all downstream dependencies have been switched over to CommonJS we can remove these.

In the process I changed the file extension of the Babel config files from .cjs to .js which results in them being formatted by Prettier like other .js files.

The browser targets of the @babel/preset-env package have also been updated to reflect the current reality [2] of what we support.

The pattern library and tests are still using the CommonJS build. We'll need to change the bundler before we can make them use the ES module build.

[1] In particular it is important that Preact is only included once in the generated app bundle. For that to happen both the app and @hypothesis/frontend-shared need to reference the same build of Preact (ES modules or CommonJS).
[2] hypothesis/client#3744

This will smooth the migration of downstream applications to a new bundler that speaks ES modules natively [1]. The existing `lib/` dir and `.babelrc` module now relate to the ES module build, and the `lib-cjs/` dir and `.babelrc-cjs` module are for the CommonJS build. Once all downstream dependencies have been switched over to CommonJS we can remove these. In the process I changed the file extension of the Babel config files from .cjs to .js which results in them being formatted by Prettier like other .js files. The browser targets of the `@babel/preset-env` package have also been updated to reflect the current reality [2] of what we support. [1] In particular it is important that Preact is only included once in the generated app bundle. For that to happen both the app and @hypothesis/frontend-shared need to reference the same build of Preact (ES modules or CommonJS). [2] hypothesis/client#3744

codecov · 2021-09-23T08:17:33Z

Codecov Report

Merging #200 (a2223b4) into main (5cd5890) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #200   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          227       227           
  Branches        78        78           
=========================================
  Hits           227       227

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 5cd5890...a2223b4. Read the comment docs.

robertknight · 2021-09-23T08:18:07Z

.babelrc.js

@@ -0,0 +1,42 @@
+'use strict';


This file is the same as .babelrc.cjs except for the changes to the @babel/preset-env plugin configuration, and reformatting with Prettier.

robertknight · 2021-09-23T08:18:44Z

.babelrc.js

+      '@babel/preset-env',
+      {
+        bugfixes: true,
+        modules: false,


modules: false is the magic that results in ES module output in lib/index.js. See https://babeljs.io/docs/en/babel-preset-env#modules.

Welp, we're not in JSON here, so you could add a helpful comment :)

robertknight · 2021-09-23T08:19:12Z

.babelrc.js

+          chrome: '70',
+          firefox: '70',
+          safari: '11.1',
+          edge: '79',


These targets have been changed to reflect hypothesis/client#3744.

robertknight · 2021-09-23T08:20:45Z

package.json

    "lib",
    "styles",
    "images"
  ],
-  "main": "./lib/index.js",
+  "main": "./lib-cjs/index.js",
+  "module": "./lib/index.js",


Modern module bundlers understand the convention that main in package.json specifies the CommonJS build of a package and the module field specifies the ES build. Browserify just ignores the new module field.

robertknight · 2021-09-23T09:01:21Z

I encountered some typechecking errors in the LMS app with this. I'll look into those next week.

lyzadanger

Whoof! This is rather brief, but dense. I left some questions.

Build is working as (I believe is) expected for me locally

Would these changes have any effect on package publishing or consumption?

Thanks for your patience with my bundler ignorances.

lyzadanger · 2021-09-23T13:12:19Z

.babelrc-cjs.js

+const presetEnvConfig = config.presets.find(
+  ([name]) => name === '@babel/preset-env'
+)[1];


This lookup is a little gross, but seems necessitated by babel config structure 🤷🏻 . It seems more fragile than it is :). I'd be curious as to the archaeology of the babel-config data structure format...

lyzadanger · 2021-09-23T13:13:21Z

.babelrc-cjs.js

@@ -0,0 +1,9 @@
+const baseConfig = require('./.babelrc');
+
+const config = { ...baseConfig };


Perhaps a helpful comment on the module level here to explain what it's doing (overriding the modules setting...etc.).

lyzadanger · 2021-09-23T13:25:10Z

package.json

-    "build": "yarn build-js && tsc --build src/tsconfig.json",
+    "build-esm": "babel src --out-dir lib/ --source-maps --ignore '**/test' --ignore '**/karma.config.js'",
+    "build-cjs": "babel src --out-dir lib-cjs/ --source-maps --ignore '**/test' --ignore '**/karma.config.js' --config-file ./.babelrc-cjs.js",
+    "build": "yarn build-cjs && yarn build-esm && tsc --build src/tsconfig.json",


Is it desired that the CJS and ESM builds will build even if tsc fails? Or might it make more sense to have the tsc command first? Thoughts?

The most important thing is that CI fails if tsc fails. Otherwise I don't think it matters too much either way.

lyzadanger · 2021-09-23T13:27:46Z

.babelrc.js

+      '@babel/preset-env',
+      {
+        bugfixes: true,
+        modules: false,


Welp, we're not in JSON here, so you could add a helpful comment :)

lyzadanger · 2021-09-23T13:34:11Z

src/karma.config.js

@@ -69,14 +69,14 @@ module.exports = function (config) {
            // The existence of this preset option is due to a config issue with where jsx modules
            // are not transpiled to js.
            // See https://github.com/hypothesis/client/issues/2929
-            presets: require('../.babelrc.cjs').presets,
+            presets: require('../.babelrc-cjs.js').presets,


Can you walk me through this a bit? The linked client issue hypothesis/client#2929 is confusing me as it references a preset-react configuration property that doesn't appear to be set explicitly by our configuration. I am having trouble following.

Also, this elects to use the CJS configuration. Would it work with the ESM variant? Can you help me understand?

On second review the comment here is out of date. The tests do need to use the CJS configuration, but that's because the current bundler (Browserify) doesn't understand ES modules. When we replace the bundler we can remove this.

We do need to configure Babel to target CommonJS in tests, but the comment explaining the import was out-dated. The current reason is because the bundler used in tests doesn't speak ES modules natively.

It's not that obvious from the name whether it refers to input or output.

robertknight · 2021-09-23T14:09:55Z

I fixed a typechecking issue in downstream projects when using a build from this branch.

There is one minor breaking change for downstream projects which use the pattern library, due to the location of the pattern library entry point having changed from lib/pattern-library/index.js to lib-cjs/pattern-library/index.js

robertknight · 2021-09-23T14:11:05Z

Would these changes have any effect on package publishing or consumption?

One minor change will be required in the client and lms apps (see #200 (comment)) but there shouldn't be any other immediate consequences.

esanzgar · 2021-09-24T07:01:25Z

.babelrc.js

+          chrome: '70',
+          firefox: '70',
+          safari: '11.1',
+          edge: '79',


I think it is better to take this out and rely on browserslist in package.json, to give consistency to all our projects.

Maybe the browsers should be similar to the list in lms not the client?
"browserslist": "chrome 57, edge 17, firefox 53, safari 10.1",

I think it is better to take this out and rely on browserslist in package.json, to give consistency to all our projects.

I agree. I'll do that in this PR.

Maybe the browsers should be similar to the list in lms not the client?

The effective browser requirements for the lms app is the maximum of the lms and client's code, since the lms app isn't useful without the client. I intend to update the client and lms app to have the same baseline as this.

It also occurred to me that since Edge is now based on Chrome, there shouldn't be a need to specify the Edge version any more. We can just treat it as being covered by Chrome.

lyzadanger · 2021-09-27T12:33:37Z

I fixed a typechecking issue in downstream projects when using a build from this branch.

There is one minor breaking change for downstream projects which use the pattern library, due to the location of the pattern library entry point having changed from lib/pattern-library/index.js to lib-cjs/pattern-library/index.js

This brings up a few questions for me, several of which are educational opportunities that I'll ping you directly about, but the salient one is: would this be a sort of temporary change in those applications until those applications' bundlers speak ESM? Or would those applications always use the CJS pattern library?

robertknight · 2021-09-28T09:59:18Z

would this be a sort of temporary change in those applications until those applications' bundlers speak ESM?

Yes. The intent is that we'll switch over all bundling in both this package and downstream to use ESM and then we can drop the CommonJS build entirely.

Move browser requirements to `browserslist` entry in package.json for consistency with other projects and to make them available to all build tools. The current Edge requirement is Edge >= 79. Since this version of Edge is a derivative of Chrome it is covered by the "chrome" entry.

lyzadanger

OK, I'm ready to try this out! Do you think you can take on merging this and publishing a new package version? I'll leave it up to you what you think a proper version bump is.

It doesn't look like much else of note has changed since the last release: v3.10.0...main

Let me know if you would like a hand!

lyzadanger · 2021-09-28T12:04:02Z

Oh! Also, the client and lms apps will need to be updated with the CJS pattern-library import. Is that something you could take (bumping those and updating the import)?

This required adapting pattern library import paths due to changes in hypothesis/frontend-shared#200.

robertknight commented Sep 23, 2021

View reviewed changes

robertknight marked this pull request as draft September 23, 2021 09:01

Base automatically changed from convert-require-to-import to main September 23, 2021 13:17

lyzadanger reviewed Sep 23, 2021

View reviewed changes

robertknight added 3 commits September 23, 2021 14:46

Revise Babel configuration in tests

7b90e7d

We do need to configure Babel to target CommonJS in tests, but the comment explaining the import was out-dated. The current reason is because the bundler used in tests doesn't speak ES modules natively.

Add a note about the meaning of the modules option

d4e9275

It's not that obvious from the name whether it refers to input or output.

Output type definitions to CommonJS build dir

305956b

robertknight marked this pull request as ready for review September 23, 2021 14:07

Reformat .babelrc-cjs.js

7109c7a

esanzgar reviewed Sep 24, 2021

View reviewed changes

robertknight added 2 commits September 28, 2021 11:02

Refine a comment about the CJS build

a2223b4

robertknight requested a review from lyzadanger September 28, 2021 10:15

lyzadanger approved these changes Sep 28, 2021

View reviewed changes

robertknight merged commit df754f0 into main Sep 29, 2021

robertknight deleted the dual-build-esm-and-cjs branch September 29, 2021 15:57

robertknight added a commit to hypothesis/client that referenced this pull request Sep 30, 2021

Update @hypothesis/frontend-shared to v3.11.0

f66aa28

This required adapting pattern library import paths due to changes in hypothesis/frontend-shared#200.

robertknight mentioned this pull request Sep 30, 2021

Update @hypothesis/frontend-shared to v3.11.0 hypothesis/client#3794

Merged

robertknight added a commit to hypothesis/client that referenced this pull request Sep 30, 2021

Update @hypothesis/frontend-shared to v3.11.0

66a30a7

This required adapting pattern library import paths due to changes in hypothesis/frontend-shared#200.

robertknight added a commit to hypothesis/lms that referenced this pull request Sep 30, 2021

Update @hypothesis/frontend-shared to v3.11.0

f738528

This required adapting pattern library import paths due to changes in hypothesis/frontend-shared#200.

robertknight mentioned this pull request Sep 30, 2021

Update @hypothesis/frontend-shared to v3.11.0 hypothesis/lms#3171

Merged

robertknight added a commit to hypothesis/lms that referenced this pull request Sep 30, 2021

Update @hypothesis/frontend-shared to v3.11.0

cdb0780

This required adapting pattern library import paths due to changes in hypothesis/frontend-shared#200.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dual-build package as ES modules and CommonJS #200

Dual-build package as ES modules and CommonJS #200

robertknight commented Sep 23, 2021 •

edited

Loading

codecov bot commented Sep 23, 2021 •

edited

Loading

robertknight Sep 23, 2021

robertknight Sep 23, 2021

lyzadanger Sep 23, 2021

robertknight Sep 23, 2021

robertknight Sep 23, 2021

robertknight commented Sep 23, 2021

lyzadanger left a comment

lyzadanger Sep 23, 2021

lyzadanger Sep 23, 2021

lyzadanger Sep 23, 2021

robertknight Sep 23, 2021

lyzadanger Sep 23, 2021

lyzadanger Sep 23, 2021

robertknight Sep 23, 2021 •

edited

Loading

robertknight commented Sep 23, 2021

robertknight commented Sep 23, 2021

esanzgar Sep 24, 2021

robertknight Sep 28, 2021

lyzadanger commented Sep 27, 2021

robertknight commented Sep 28, 2021

lyzadanger left a comment

lyzadanger commented Sep 28, 2021

		@@ -0,0 +1,9 @@
		const baseConfig = require('./.babelrc');

		const config = { ...baseConfig };

Dual-build package as ES modules and CommonJS #200

Dual-build package as ES modules and CommonJS #200

Conversation

robertknight commented Sep 23, 2021 • edited Loading

codecov bot commented Sep 23, 2021 • edited Loading

Codecov Report

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

robertknight commented Sep 23, 2021

lyzadanger 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

robertknight Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

robertknight commented Sep 23, 2021

robertknight commented Sep 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzadanger commented Sep 27, 2021

robertknight commented Sep 28, 2021

lyzadanger left a comment

Choose a reason for hiding this comment

lyzadanger commented Sep 28, 2021

robertknight commented Sep 23, 2021 •

edited

Loading

codecov bot commented Sep 23, 2021 •

edited

Loading

robertknight Sep 23, 2021 •

edited

Loading