Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framework: Add lerna to publish @automattic/simple-components (Take Two) #24788

Closed

Conversation

ockham
Copy link
Contributor

@ockham ockham commented May 10, 2018

Add lerna (a monorepo tool) to Calypso, and use it to publish @automattic/simple-components as a separate npm.

This involves adding a package.json, .gitignore, and .npmignore to client/components, and an alias field to Calypso's webpack configs.

Since we want Calypso (and possibly other consumers) to continue to use untranspiled code (to benefit from features such as tree-shaking), the npms we produce should contain both transpiled and untranspiled versions of the code. See http://2ality.com/2017/07/npm-packages-via-babel.html, and for more information, http://2ality.com/2017/04/transpiling-dependencies-babel.html and http://2ality.com/2017/06/pkg-esnext.html.

Please note #16782 (comment)!

To test:

  • Run npm run distclean && npm start
  • Verify that Calypso works
  • Note that node_modules/@automattic/simple-components is a symlink to client/components/!
  • Also verify that building (npm run build-docker) and running (npm run docker) the Docker container still works (see PCYsg-5XR-p2).

To publish the npm (only if you know what you're doing! 😛):

  • Run npx lerna publish --skip-git
    • If you have 2fa (in auth-and-writes mode) enabled for your npm account, you need to prepend NPM_CONFIG_OTP=<your2FAcode> to that command -- and be quick before it expires while attempting to publish! Related: Support 2FA --otp=123456 lerna/lerna#1091
  • When asked for version, select Prerelease for @automattic/simple-components!)

Related (but not required for this PR!): #19698.
Take One was #16782.

@matticbot
Copy link
Contributor

@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo-take-two branch 2 times, most recently from 5ab80af to c76a26b Compare May 10, 2018 12:33
@ockham ockham changed the title Framework: Add lerna to publish @automattic/simple-components (Take Two( Framework: Add lerna to publish @automattic/simple-components (Take Two) May 10, 2018
lerna.json Outdated
@@ -0,0 +1,7 @@
{
"lerna": "2.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.11.0 according to package? (latest version)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot!

package.json Outdated
"docker": "docker run -it --name wp-calypso --rm -p 80:3000 wp-calypso",
"env": "cross-env-shell NODE_PATH=$NODE_PATH:server:client:.",
"eslint-branch": "node bin/eslint-branch.js",
"install-if-deps-outdated": "node bin/install-if-deps-outdated.js",
"install-if-no-packages": "node bin/install-if-no-packages.js",
"postinstall": "npx lerna bootstrap --hoist",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should move it to prebuild...

@blowery
Copy link
Contributor

blowery commented May 23, 2018

would this still work with greenkeeper? greenkeeperio/greenkeeper#139

@ockham
Copy link
Contributor Author

ockham commented Jun 2, 2018

would this still work with greenkeeper? greenkeeperio/greenkeeper#139

This looks promising: greenkeeperio/greenkeeper#139 (comment)

@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo-take-two branch from c6d7b65 to 8b44db8 Compare June 2, 2018 12:17
@blowery
Copy link
Contributor

blowery commented Jun 6, 2018

@ockham we're going to try out renovate instead of greenkeeper. see #25308. it has baked in monorepo support, so 🤞

@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo-take-two branch 2 times, most recently from d77849b to d588545 Compare July 16, 2018 10:55
@blowery
Copy link
Contributor

blowery commented Jul 16, 2018

Since we want Calypso (and possibly other consumers) to continue to use untranspiled code (to benefit from features such as tree-shaking), the npms we produce should contain both transpiled and untranspiled versions of the code.

I'm not sure we want to publish untranspiled code, ever. That pushes the burden of transpiling and the associated babel config down to a client, who may be running a different version of babel with different presets.

I think publishing es5 with modules is likely fine, this is what lodash and a bunch of others are doing with the module flag on package.json, but I'm not sure how untranspiled modules would work.

"version": "0.1.0-alpha.5",
"description": "React components, as used on WordPress.com",
"scripts": {
"build": "npx babel --only ./index.js,./button/,./main/ --out-dir cjs .",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the babel 7 beta 53 upgrade, this is going to get more complicated. babel will now pull the config from the root of the repo, in babel.config.js instead of looking for the closest .babelrc.js. So far we don't have a .babelrc.js, but we may want one.

How does this package handle polyfills? We're generating them at the entry point in calypso, but for a package like this, we probably want them polyfill'd at the usage point? Or not at all and we require the client to polyfill what we need?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this package handle polyfills?

Badly 😆 Just been struggling with the following error

ERROR in ./client/boot/polyfills.js
Module not found: Error: Can't resolve 'core-js/modules/es6.array.sort' in '~/src/wp-calypso/client/boot'

So yeah, generating at entry point is probably the least desirable...

Or not at all and we require the client to polyfill what we need?

That seems kinda reasonable...

Copy link
Contributor Author

@ockham ockham Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just filed #26094 which should fix the polyfill issue.

],
"dependencies": {
"classnames": "2.2.6",
"lodash": "4.17.10",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we make sure these don't get out of sync with calypso proper? They should probably be ^ revs like ^4.17.10, as this is a library, not an app.

Copy link
Member

@simison simison Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a wrapper script that makes sure versions are identical across Calypso + modules during pre-push.

Idea from:
https://github.com/wix/lerna-script/tree/master/tasks/modules & https://youtu.be/MCOmdxJ6qTU

@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo-take-two branch from 89043a2 to aaf4c51 Compare July 16, 2018 22:42
@ockham ockham mentioned this pull request Jul 16, 2018
2 tasks
@coderkevin
Copy link
Contributor

I'm not sure we want to publish untranspiled code, ever. That pushes the burden of transpiling and the associated babel config down to a client, who may be running a different version of babel with different presets.

There's a precedent in several npm packages to publish with multiple copies of the same code, including commonjs, minified bundle, and even the original es2015+ code. While I agree that the package.json main should be the commonjs implementation at this point, there's no harm in giving people options--especially if we document which babel options are required to transpile.

},
"homepage": "https://github.com/Automattic/wp-calypso/blob/master/docs/components.md",
"main": "./cjs/index.js",
"esnext": "./index.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mirror the Gutenberg lerna config (build scripts etc...)?

We do publish two versions per module as well:

  • main which corresponds to the transpiled version
  • module which is a transpiled version but still using ES6 modules which guarantee tree shaking etc...

Is it a common usage to use an esnext property for untranspiled code? Also, how do we (package consumers) know what babel config or transpiling is needed with this field if it's not normalized?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on using main and module as you said.

the latest i've seen on publishing es6+ is https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See react-router as an example:
https://unpkg.com/react-router@4.3.1/package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mirror the Gutenberg lerna config (build scripts etc...)?

I didn't want to necessarily introduce build scripts yet since that seemed overblown for the little amount of transpilation we're doing at this point (YAGNI -- might add them later as needed tho).

We do publish two versions per module as well:

  • main which corresponds to the transpiled version
  • module which is a transpiled version but still using ES6 modules which guarantee tree shaking etc...

Is it a common usage to use an esnext property for untranspiled code? Also, how do we (package consumers) know what babel config or transpiling is needed with this field if it's not normalized?

I picked up esnext from the 2ality blog, see PR desc:

http://2ality.com/2017/07/npm-packages-via-babel.html, and for more information, http://2ality.com/2017/04/transpiling-dependencies-babel.html and http://2ality.com/2017/06/pkg-esnext.html.

Granted, this is far from standardized, so I'm happy to reconsider.

@youknowriad youknowriad requested a review from gziolo July 17, 2018 12:44
@blowery
Copy link
Contributor

blowery commented Jul 17, 2018

Some good reading on the possible future of transpiling dependencies https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages

@gziolo
Copy link
Member

gziolo commented Jul 17, 2018

If you want to get closer to the setup Gutenberg uses, you can use this tool:
https://www.npmjs.com/package/@wordpress/scripts#wp-scripts-lint-pkg-json

You can also override some config values provided by default using:
https://github.com/WordPress/gutenberg/blob/master/package.json#L126-L145

"version": "0.1.0-alpha.6",
"description": "React components, as used on WordPress.com",
"scripts": {
"build": "npx babel --only ./index.js,./badge/,./button/,./main/,./notice/ --out-dir cjs .",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's super simple when you compare to what we offer in Gutenberg:
https://github.com/WordPress/gutenberg/blob/master/bin/packages/build.js

We might want to sync on this and expose one general script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like simple 😁 TBH, those build scripts looked a bit... overblown? 😁 To me, it seemed like the most relevant config options were (declarative) .babelrc material (rather than -- imperative -- script material). I think I'd like us at least to wait for the Babel 7-beta.53 upgrade (#26020) to see how far we can get using Babel configs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our build scripts are inspired by Jest and they are built the way they are mainly because we want quick "watch" support, which means if there's a change in a file we only transpile this file. Maybe it's possible to simplify this as well but yeah let's try to align once the project matures.

@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo-take-two branch 2 times, most recently from 4a7d3a5 to 8f1e0ee Compare July 17, 2018 20:56
@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo-take-two branch 2 times, most recently from a9a6e0d to 6599b3c Compare July 19, 2018 12:18
@jsnajdr
Copy link
Member

jsnajdr commented Jul 19, 2018

I pushed a fix for the bug where core-js polyfills failed to load.

As core-js is not a direct dependency in package.json, we rely on it being promoted to the top-level node_modules directory. The latest version is 2.5.7.

But there is package fbjs that depends on 1.x version. The jsx-to-string package your branch added depends on fbjs, too. Suddenly, core-js@1.x started winning the race to the top.

The transpiled @babel/polyfill suddenly tries to import modules from core-js@1.x which misses a lot of recent es7 polyfills. Crash!

Fixed by adding core-js@2.5.7 as a top-level Calypso dependency. Also recommended (in a very obscure way though) in Babel docs.

I also pushed a revert of #26094, as it's no longer interesting.

ockham added a commit that referenced this pull request Jul 20, 2018
)" (#26192)

This reverts commit ed6beaa.

[Rationale](#26094 (comment)):

> This PR [increased](http://iscalypsofastyet.com/push/ed6beaa5e0b54f6ddbe11360c8ad8a572b83a32f/e3642614723d88c353e5d768969d8f877d7472c4) the bundle size and [reverted](http://iscalypsofastyet.com/push/acef9fa493cc06a9fefece7ff00759997af8ca8a/03502467b044464e191c9746c510c6a10a7930e0) the improvements achieved in #25961.
>
> It's because `babel-preset-env` used to transform the `@babel/polyfill` import into a list that excludes the polyfills that are not needed on the target browser list. In the new location of `@babel/polyfill`, the plugin doesn't get the opportunity to do that transform and all polyfills are bundled again.

I filed #26094 in the first place to get #24788 to work. Fortunately, @jsnajdr found [another way](#24788 (comment)).
jsnajdr and others added 2 commits July 20, 2018 20:08
Pins the version in top-level `node_modules` to 2.5.7, not a random version that happens to be
promoted by NPM Install to the top-level directory.

Fixes a bug where `import from 'core-js'` statements generated by `babel-preset-env` started using
an old 1.x version suddenly.
Add `lerna` (a monorepo tool) to Calypso, and use it to publish [`@automattic/simple-components`](https://www.npmjs.com/package/@automattic/simple-components) as a separate npm.

This involves adding a `package.json`, `.gitignore`, and `.npmignore` to `client/components`, and an `alias` field to Calypso's webpack configs.

Since we want Calypso (and possibly other consumers) to continue to use untranspiled code (to benefit from features such as tree-shaking), the npms we produce should contain both transpiled and untranspiled versions of the code. See http://2ality.com/2017/07/npm-packages-via-babel.html, and for more information, http://2ality.com/2017/04/transpiling-dependencies-babel.html and http://2ality.com/2017/06/pkg-esnext.html.
@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo-take-two branch from ec181d0 to 850c496 Compare July 20, 2018 18:12
@@ -0,0 +1 @@
node_modules
Copy link
Member

@simison simison Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ockham
Copy link
Contributor Author

ockham commented Oct 15, 2018

Closing. We're using lerna as of #27356. This has served its purpose as an exploratory branch.

@ockham ockham closed this Oct 15, 2018
@ockham ockham deleted the add/lerna-to-publish-form-components-repo-take-two branch October 15, 2018 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants