Build packages using Rollup #10386

swissspidy · 2022-01-25T23:10:51Z

Context

Before we can publish our packages on npm we need a build step for them, ideally to produce both CommonJS and ESM versions.

Summary

This is an attempt at this by leveraging Rollup and several Rollup plugins that fit our needs.

Building packages creates dist and dist-module folders that are then used for distribution.

The src folder is entirely ignored for publishing on npm.

Basic usage:

# Build all packages using Rollup
$ npm run bundle

# Build only the migration package
$ npm run bundle -- --configPackages=migration

# Build only the module version of all packages
$ npm run bundle -- --configEntries=es

# Build only the CommonJS version of the migration and text-sets packages
$ npm run bundle -- --configEntries=cjs --configPackages=migration,text-sets

Relevant Technical Choices

Decided to try Rollup because we're already using it in the project and it was relatively easy to get off the ground.

Changed some imports to avoid circular dependencies, though this still needs more work.

Changed some imports to avoid mixing default imports and named exports. Quote:

It is bad practice to mix default and named exports in the same module, though it is allowed by the specification.

Used custom resolvers for webpack and Jest so they continue to look for packages in the src folders instead of dist.

To-do

Testing
Fix inline web worker usage in story-editor
Fix import/no-unresolved false positives
Add sourcemaps?
Fix duplicate React version in react-photo-gallery
Fix some circular dependencies (see below)

(!) Circular dependencies
packages/story-editor/src/masks/index.js -> packages/story-editor/src/elements/index.js -> packages/story-editor/src/elements/elementTypes.js -> packages/story-editor/src/elements/text/index.js -> packages/story-editor/src/elements/text/display.js -> packages/story-editor/src/elements/shared/index.js -> packages/story-editor/src/utils/elementBorder.js -> packages/story-editor/src/masks/index.js

packages/story-editor/src/app/story/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/combineElements.js -> packages/story-editor/src/masks/index.js -> packages/story-editor/src/elements/index.js -> packages/story-editor/src/elements/elementTypes.js -> packages/story-editor/src/elements/text/index.js -> packages/story-editor/src/elements/text/edit.js -> packages/story-editor/src/app/story/index.js

packages/story-editor/src/app/story/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/combineElements.js -> packages/story-editor/src/masks/index.js -> packages/story-editor/src/elements/index.js -> packages/story-editor/src/elements/elementTypes.js -> packages/story-editor/src/elements/text/index.js -> packages/story-editor/src/elements/text/frame.js -> packages/story-editor/src/app/story/index.js

...and 48 more

User-facing changes

None

Testing Instructions

This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

Verify that all status checks pass
Run npm run bundle to build the packages yourself

Reviews

Does this PR have a security-related impact?

Not that I could think of

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

No

Checklist

This PR addresses an existing issue and I have linked this PR to it in ZenHub
I have tested this code to the best of my abilities
I have verified accessibility to the best of my abilities (docs)
I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
I have added documentation where necessary
I have added a matching Type: XYZ label to the PR

See #10251

packages/dashboard/src/index.js

mohdsayed · 2022-01-26T14:05:22Z

rollup.config.js

+    dedupe: [],
+  }),
+  babel({
+    babelHelpers: 'inline',


Trying to learn why inline is used as an option for babelHelpers here

I used runtime initially as it was the recommendation, but I got some warnings when building.

mohdsayed · 2022-01-26T14:56:07Z

(!) Circular dependencies
packages/story-editor/src/masks/index.js -> packages/story-editor/src/elements/index.js -> packages/story-editor/src/elements/elementTypes.js -> packages/story-editor/src/elements/text/index.js -> packages/story-editor/src/elements/text/display.js -> packages/story-editor/src/elements/shared/index.js -> packages/story-editor/src/utils/elementBorder.js -> packages/story-editor/src/masks/index.js
packages/story-editor/src/app/story/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/combineElements.js -> packages/story-editor/src/masks/index.js -> packages/story-editor/src/elements/index.js -> packages/story-editor/src/elements/elementTypes.js -> packages/story-editor/src/elements/text/index.js -> packages/story-editor/src/elements/text/edit.js -> packages/story-editor/src/app/story/index.js
packages/story-editor/src/app/story/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/index.js -> packages/story-editor/src/app/story/useStoryReducer/reducers/combineElements.js -> packages/story-editor/src/masks/index.js -> packages/story-editor/src/elements/index.js -> packages/story-editor/src/elements/elementTypes.js -> packages/story-editor/src/elements/text/index.js -> packages/story-editor/src/elements/text/frame.js -> packages/story-editor/src/app/story/index.js
...and 48 more

51 indirect circular dependencies are a lot to resolve. 😵‍💫

mohdsayed · 2022-01-26T15:33:29Z

I think we may ignore these circular dependency warning for now by using onwarn and deal with them later separately.

onwarn(warning, warn) {
  if (warning.code !== 'CIRCULAR_DEPENDENCY') {
    warn(warning);
  }
}

swissspidy · 2022-01-26T15:47:22Z

They don't cause the build to error though, so not a big deal to keep them visible and remind os of this technical debt :)

mohdsayed · 2022-01-26T16:05:34Z

They don't cause the build to error though, so not a big deal to keep them visible and remind os of this technical debt :)

Yes, maybe a lot of them will be resolved automatically when we break down the story-editor in more packages in the upcoming package organization PRs.

mohdsayed · 2022-01-26T18:14:10Z

@swissspidy Can you explain this one a bit from the to-do list?

Fix import/no-unresolved false positives

swissspidy · 2022-01-26T20:29:24Z

Rollup doesn't seem to have support for Web Workers, see rollup/rollup#3842

Tried rollup-plugin-web-worker-loader npm package however it breaks with the following error

I'm inclined to just ditch our current web worker logic completely. Not a huge fan of the webpack worker-loader anyway.

We could take a very simple approach by manually turning it into an inline/embedded web worker (see examples at https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers#embedded_workers) using a blob — which is basically what worker-loader is doing anyway — and what rollup-plugin-web-worker-loader should be supporting as well.

So something like this:

function generateBlurHash() {
// ...
}

const blob = new Blob(['('+generateBlurHash.toString()+')()'], {type: 'text/javascript'})
const workerUrl = URL.createObjectURL(blob);

new Worker(workerUrl)

At least for the short term this should do the trick.

Suggestions welcome!

@swissspidy Can you explain this one a bit from the to-do list?

Fix import/no-unresolved false positives

Sure.

Basically right now we have tons of ESLint warnings here on this PR.

They are caused by all the changes I did to the package.json files for all the packages (specifically, changing the main field).

They come from eslint-plugin-import, specifically the no-unresolved rule

It uses Node-style module resolution by default, which has some open issues about this

import-js/eslint-plugin-import#1868
import-js/eslint-plugin-import#1810

Perhaps using eslint-import-resolver-webpack fixes this?

Otherwise I suppose we can just disable this rule until the above issues get fixed.

mohdsayed · 2022-01-27T09:14:36Z

Great idea of turning into an inline/embedded web worker using blob 👍

For Eslint warnings, I tried eslint-import-resolver-webpack, but that didn't work for me.

Gutenberg also had a similar issue here WordPress/gutenberg#31792 for which they used eslint-import-resolver-node, see /.eslintrc.js#L53

packages/i18n/package.json

rollup.config.js

packages/animation/package.json

mohdsayed · 2022-01-27T15:32:46Z

Tested it after manually merging #10297 and

In package.json

Updated "module": "./dist-module/index.js" to "module": "dist-module/index.js"
Removed "type": "module"
Removed "exports" ( don't know why but it imports from ./src directory when leaving this in package.json )

I think all import issues were resolved but now we have some node errors.

ERROR in ./node_modules/@googleforcreators/story-editor/dist-module/index-276f5876.js
Module not found: Error: Can't resolve './raw' in '/Users/sayedtaqui/playground/react-app/node_modules/@googleforcreators/story-editor/dist-module'
 @ ./node_modules/@googleforcreators/story-editor/dist-module/index-276f5876.js 404:624305-624330
 @ ./node_modules/@googleforcreators/story-editor/dist-module/index.js
 @ ./src/index.js

ERROR in ./node_modules/@googleforcreators/story-editor/dist-module/index-276f5876.js
Module not found: Error: Can't resolve './raw' in '/Users/sayedtaqui/playground/react-app/node_modules/@googleforcreators/story-editor/dist-module'
 @ ./node_modules/@googleforcreators/story-editor/dist-module/index-276f5876.js 136:46199-46219
 @ ./node_modules/@googleforcreators/story-editor/dist-module/index.js
 @ ./src/index.js

ERROR in ./node_modules/@googleforcreators/story-editor/dist-module/index-c3e659ac.js
Module not found: Error: Can't resolve 'fs' in '/Users/sayedtaqui/playground/react-app/node_modules/@googleforcreators/story-editor/dist-module'
 @ ./node_modules/@googleforcreators/story-editor/dist-module/index-c3e659ac.js 17:99-117 17:24780-24781
 @ ./node_modules/@googleforcreators/story-editor/dist-module/index-276f5876.js
 @ ./node_modules/@googleforcreators/story-editor/dist-module/index.js
 @ ./src/index.js

package.json

packages/jest-resolver/package.json

Followup to #10386

swissspidy added Type: Infrastructure Changes impacting testing infrastructure or build tooling Pod: WP & Infra labels Jan 25, 2022

swissspidy requested a review from mohdsayed January 25, 2022 23:10

This comment was marked as resolved.

Sign in to view

mohdsayed reviewed Jan 26, 2022

View reviewed changes

packages/dashboard/src/index.js Show resolved Hide resolved

mohdsayed reviewed Jan 26, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

mohdsayed reviewed Jan 27, 2022

View reviewed changes

packages/i18n/package.json Outdated Show resolved Hide resolved

mohdsayed reviewed Jan 27, 2022

View reviewed changes

rollup.config.js Show resolved Hide resolved

mohdsayed reviewed Jan 27, 2022

View reviewed changes

packages/animation/package.json Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

swissspidy added 4 commits January 27, 2022 17:41

Try to reduce some circular dependencies

2a048c1

Remove web worker code for now, pending todo

6b38d9d

Avoid mixing named and default exports

5de0dfc

First attempt at building with Rollup

e5b87ea

mohdsayed reviewed Jan 30, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

swissspidy added 2 commits January 31, 2022 11:42

Use jest resolver for e2e tests

daae04f

Update script

9de0b1a

This comment has been minimized.

Sign in to view

swissspidy added 8 commits January 31, 2022 12:20

Templates: Add file extension to dynamic imports

d3f58cd

Change plugin order in rollup config

d717523

Allow building only specified entries

8a8b012

Specify runOnce

49c8ec9

Tweak Terser options

f462acd

Yolo: Try marking all deps as external

5c432a0

Simply ignore src folder in npmignore

3524e41

Merge branch 'main' into add/10251-rollup

c1726e6

This comment has been minimized.

Sign in to view

swissspidy added 6 commits February 1, 2022 11:51

Merge branch 'main' into add/10251-rollup

c515a02

Try using rollup-plugin-web-worker-loader

2fdb291

Merge branch 'main' into add/10251-rollup

878685f

Add bundle step to workflow for debugging

86cea29

Lint fix

9aadfd9

Increase timeout for lint i18n step

05d183c

swissspidy changed the title ~~[TRY] [WIP] Build packages using Rollup~~ Build packages using Rollup Feb 3, 2022

Merge branch 'main' into add/10251-rollup

4947ca9

swissspidy marked this pull request as ready for review February 3, 2022 08:37

swissspidy requested a review from spacedmonkey as a code owner February 3, 2022 08:37

swissspidy requested review from mohdsayed and removed request for spacedmonkey February 3, 2022 08:37

mohdsayed reviewed Feb 3, 2022

View reviewed changes

packages/jest-resolver/package.json Show resolved Hide resolved

mohdsayed approved these changes Feb 3, 2022

View reviewed changes

swissspidy merged commit db9773a into main Feb 3, 2022

swissspidy deleted the add/10251-rollup branch February 3, 2022 21:23

swissspidy added a commit that referenced this pull request Feb 4, 2022

Fix module resolution in storybook

04f0d31

Followup to #10386

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build packages using Rollup #10386

Build packages using Rollup #10386

swissspidy commented Jan 25, 2022 •

edited

Loading

This comment was marked as resolved.

mohdsayed Jan 26, 2022

swissspidy Jan 26, 2022

mohdsayed commented Jan 26, 2022 •

edited

Loading

mohdsayed commented Jan 26, 2022

swissspidy commented Jan 26, 2022

mohdsayed commented Jan 26, 2022

This comment was marked as resolved.

mohdsayed commented Jan 26, 2022 •

edited

Loading

swissspidy commented Jan 26, 2022

mohdsayed commented Jan 27, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mohdsayed commented Jan 27, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Build packages using Rollup #10386

Build packages using Rollup #10386

Conversation

swissspidy commented Jan 25, 2022 • edited Loading

Context

Summary

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

Reviews

Does this PR have a security-related impact?

Does this PR change what data or activity we track or use?

Does this PR have a legal-related impact?

Checklist

This comment was marked as resolved.

mohdsayed Jan 26, 2022

Choose a reason for hiding this comment

swissspidy Jan 26, 2022

Choose a reason for hiding this comment

mohdsayed commented Jan 26, 2022 • edited Loading

mohdsayed commented Jan 26, 2022

swissspidy commented Jan 26, 2022

mohdsayed commented Jan 26, 2022

This comment was marked as resolved.

mohdsayed commented Jan 26, 2022 • edited Loading

swissspidy commented Jan 26, 2022

mohdsayed commented Jan 27, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mohdsayed commented Jan 27, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

swissspidy commented Jan 25, 2022 •

edited

Loading

mohdsayed commented Jan 26, 2022 •

edited

Loading

mohdsayed commented Jan 26, 2022 •

edited

Loading