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 #16782

Closed
wants to merge 6 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Aug 1, 2017

Updated -- no longer requires lerna to be installed globally!

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, run lerna publish --skip-git (but don't unless you know what you're doing!) When asked for version, select Prerelease for @automattic/simple-components!)

Related (but no longer required for this PR!): #16870.

@ockham ockham self-assigned this Aug 1, 2017
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Aug 1, 2017
@matticbot
Copy link
Contributor

@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo branch 2 times, most recently from 454ed82 to b78e15a Compare August 1, 2017 14:56
"version": "0.1.0-alpha.1",
"description": "React components, as used on WordPress.com",
"scripts": {
"build": "babel ./index.js --out-dir cjs && babel ./button --out-dir cjs/button && babel ./main --out-dir cjs/main",
Copy link
Contributor Author

@ockham ockham Aug 3, 2017

Choose a reason for hiding this comment

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

Noting here that I also tried

babel --only ./index.js,./button/,./main/ --out-dir cjs .

but that is unfortunately evaluated as a glob (and apparently broken) since it includes e.g. every path that ends in button/ or main/, and every index.js[x] (as opposed to just top-level ones).

Babel 7 changes --only and --ignore args to be evaluated as relative paths instead of globs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might want to consider moving components that are supposed to go into simple-components into a separate subfolder, e.g. client/components/simple. This is going to save us a lot of hassle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I should probably file an issue against Babel 6, since this behavior doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I should probably file an issue against Babel 6, since this behavior doesn't seem right.

babel/babel#6067

@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo branch from 3c15803 to 481c294 Compare August 3, 2017 15:11
@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo branch 3 times, most recently from b827025 to 152a883 Compare August 7, 2017 10:36
@ockham ockham changed the title Framework: Add lerna Framework: Add lerna to publish @automattic/simple-components Aug 7, 2017
@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo branch from 5724135 to 94cd250 Compare August 7, 2017 13:39
@ockham ockham requested review from nb and coderkevin August 7, 2017 13:40
@ockham
Copy link
Contributor Author

ockham commented Aug 7, 2017

Okay, I'd say this is ready for review!

@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Aug 7, 2017
package.json Outdated
@@ -250,6 +251,7 @@
"glob": "7.0.3",
"husky": "0.13.3",
"jscodeshift": "0.3.30",
"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.

Why the ^s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good spot. Comes from running lerna init. I'll remove the caret.

Copy link
Member

Choose a reason for hiding this comment

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

Same for @automattic/simple-components.

@samouri
Copy link
Contributor

samouri commented Aug 10, 2017

@gwwar: you need to run npm install -g lerna

@gwwar
Copy link
Contributor

gwwar commented Aug 10, 2017

Summary says:

Updated -- no longer requires lerna to be installed globally!

@sirreal
Copy link
Member

sirreal commented Aug 11, 2017

I'd be happy to make lerna a local dep. I'm able to clone the repo fresh and run npm start, everything boots and runs correctly 🎉

Running this locally for me, it can't find lerna

It's normal that when lerna isn't installed distclean won't work. Similar to how npm run distclean && npm run distclean will complete on the first and fail on the second after deps have been installed via npm start. Looks like we need another solution for that one.

package.json Outdated
@@ -194,12 +195,13 @@
"clean:public": "npm run -s rm -- public .css .css.map .js .js.map",
"predashboard": "npm run -s install-if-deps-outdated",
"dashboard": "cross-env DASHBOARD=1 webpack-dashboard -- npm start",
"distclean": "npm run -s clean && npm run -s rm -- node_modules",
"distclean": "npm run -s clean && lerna clean --yes && npm run -s rm -- node_modules",
Copy link
Member

Choose a reason for hiding this comment

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

This is problematic as lerna is not global and nothing guarantees it will be present.

Copy link
Member

Choose a reason for hiding this comment

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

No problem here, npm is adding node_modules/.bin when running scripts.

Copy link
Member

Choose a reason for hiding this comment

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

Well, there is a problem if it’s not present, of course.

@ockham
Copy link
Contributor Author

ockham commented Aug 11, 2017

Fixed by replacing the && with a ;. Think that's a good enough solution?

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I think this is in a good place and ready to move forward. Thanks for your work on it and patience 🙂

@sirreal
Copy link
Member

sirreal commented Aug 14, 2017

I just realized something and wanted to share it here. When we get to Node 8 LTS (mid-october) it will include npm@^5.2 which includes npx, which will automagially local-install required binaries, and we won't have to worry about whether lerna is present locally/globall in these scripts, just s/npm/npx/ 🎉

package.json Outdated
@@ -194,12 +195,13 @@
"clean:public": "npm run -s rm -- public .css .css.map .js .js.map",
"predashboard": "npm run -s install-if-deps-outdated",
"dashboard": "cross-env DASHBOARD=1 webpack-dashboard -- npm start",
"distclean": "npm run -s clean && npm run -s rm -- node_modules",
"distclean": "npm run -s clean && lerna clean --yes ; npm run -s rm -- node_modules",
Copy link
Contributor

@DanReyLop DanReyLop Aug 29, 2017

Choose a reason for hiding this comment

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

Please use && instead of ;, unless for some reason lerna clean returns a non-zero status on completion.

Nevermind, I've just read the PR comments. Sorry for the noise.

@alisterscott
Copy link
Contributor

Is this one still going ahead? Thanks

@ockham
Copy link
Contributor Author

ockham commented Oct 31, 2017

Still relevant/being discussed, yeah. Would prefer to keep it open for another while.

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 branch from 875d43e to 9b0f1b8 Compare October 31, 2017 20:46
@ockham ockham force-pushed the add/lerna-to-publish-form-components-repo branch from 9b0f1b8 to e0a31d1 Compare October 31, 2017 20:46
@apeatling
Copy link
Member

@ockham Let's give this another 2-3 weeks, if there's no movement I think it's time to move on.

@kwight
Copy link
Contributor

kwight commented Dec 6, 2017

Any verdict here @ockham of how or whether to proceed?

@ockham
Copy link
Contributor Author

ockham commented Dec 7, 2017

Sigh, I haven't found time in the past weeks to reinvigorate the conversation around this idea. So I guess I'll close it 🙁

@ockham ockham closed this Dec 7, 2017
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 7, 2017
@alisterscott alisterscott deleted the add/lerna-to-publish-form-components-repo branch February 1, 2018 06:37
@ockham ockham restored the add/lerna-to-publish-form-components-repo branch May 10, 2018 10:36
@ockham ockham deleted the add/lerna-to-publish-form-components-repo branch May 10, 2018 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Components Framework Marked for Review Day [Size] XL Probably needs to be broken down into multiple smaller issues [Status] Needs Rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants