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

Primer monorepo #230

Merged
merged 98 commits into from
Jun 23, 2017
Merged

Primer monorepo #230

merged 98 commits into from
Jun 23, 2017

Conversation

broccolini
Copy link
Member

@broccolini broccolini commented May 11, 2017

Moving Primer into a monorepo

This pr works towards moving the source files for Primer back into the Primer org, to be pulled into GitHub via npm. More context on this decision here, and here's an interesting post on why Babel is a monorepo.

  • All packages will live in one repo to make it easier to make updates across Primer
  • Individual packages and meta-packages will still be independently versioned and distributed via npm

To-do:

  • Remove git logs from packages
  • Add ignore for npm-debug.log (and remove any existing logs)
  • Remove individual module contributing guidelines and add one to the root directory
  • Update git and issue urls to point to primer-css
  • Setup Lerna for easier multi-package management
  • Check tests still work (esp primer-module-build)
  • Update Travis config
  • Create pre-release for testing with new style guide docs and GitHub
  • Switch GitHub over to pulling in primer, remove and re-org stylesheets directory https://github.com/github/github/pull/74495

Post-ship to-do:

  • Add installation instructions for design-systems so that they can run Lerna etc. and publish releases
  • Figure out how to manage global and individual package and meta-package changelogs (perhaps using lerna/changelog)
  • Setup github releases (perhaps using zeit/release)
  • Deprecate old packages and their repos
  • Add doc explaining decision for mono repo (like Babel does)
  • Explore using Semantic Release to help us better manage versions and the changelog (I think this is low priority but would like to see what @jonrohan and Shawn think once we've used the new setup with Lerna for a while)

Questions:

  • Should each package and meta-package have it's own .gitignore file or just the root? - yes
  • Should each package and meta-package have it's own LICENSE or just the root? - yes

@primer/design-systems @muan @josh @mislav What other things do I need to take into account? Any questions, concerns? Let me know if you have any thoughts on using Lerna and Semantic Release—they look like good options and help us with some of our maintenance and publishing workflow issues. I'll need help once this is ready with switching GitHub over to pulling this in a dependency.

@broccolini broccolini self-assigned this May 11, 2017
@muan
Copy link
Contributor

muan commented May 23, 2017

So excited for the new setup! ✨

  • Should each package and meta-package have it's own .gitignore file or just the root?
  • Should each package and meta-package have it's own LICENSE or just the root?

I think they should have their own ones since each package can be installed separately. User will need both files when they are using just one package.

[ ] Setup Lerna for easier multi-package management

👍 on this

[ ] Explore using Semantic Release to help us better manage versions and the changelog

IMO this doesn't give us huge wins since lerna already prompts us to enter a version number so this is already a deliberate + essential action.

[ ] Create pre-release for testing with new style guide docs and GitHub

With normal one-to-one repo/npm packages, you would be able to just install from source without a release. Like npm install github:muan/romanize-names#test-branch, this was the way I was suggesting to do testing with. I wasn't able to find an example like this for lerna. Have you come across one? is pre-release the only way?

Also it looks to me that packages aren't versioned individually for React, Babel, and Jest, and I can't find package.json for each package in Angular, Meteor, or Ember. Do you know of an example repo somewhere?

@broccolini
Copy link
Member Author

Thanks @muan for the feedback! Replies below but also going to follow up with a comment re breaking this up a bit since not everything is essential for the first ship to make this a mono-repo.

I think they should have their own ones since each package can be installed separately. User will need both files when they are using just one package.

🆒

IMO this doesn't give us huge wins since lerna already prompts us to enter a version number so this is already a deliberate + essential action.

The value of semantic release seems to be removing the human element from making decisions about version changes. Folks incorrectly bumping the version number has been a continuous issue, I think due to a combination of personal opinion, being unfamiliar with Semver, or because mapping CSS changes to Semver isn't always obvious. I'm going to leave this til later though as I agree that it's not essential to shipping this pr, and Lerna will stop us bumping a minor or major number and forgetting to reset the patch to 0. 😬

With normal one-to-one repo/npm packages, you would be able to just install from source without a release. Like npm install github:muan/romanize-names#test-branch, this was the way I was suggesting to do testing with. I wasn't able to find an example like this for lerna. Have you come across one? is pre-release the only way?

I'd like to chat with you about this. I suggested this since I thought was a normal/best practice since it keeps the distribution via npm and allows you to share with the open-source community for testing. Maybe this isn't necessary for us, but want to make sure it's easy for all GitHub teams using Primer. This will increase over the next year as more non-dotcom sites move over to Primer so whatever we do we need to make sure it's easy to communicate and test updates.

Also it looks to me that packages aren't versioned individually for React, Babel, and Jest, and I can't find package.json for each package in Angular, Meteor, or Ember. Do you know of an example repo somewhere?

Not without spending some time searching around, no. But it doesn't matter to me that those projects aren't individually versioning their packages, it's a supported feature of Lerna and seems like the right thing to do for us, and it maintains our current versioning system. It makes it easier to push individual updates more safely since folks using it will be able to specify what they're ready to update to. I.e. if we updated our button styles and our box styles, but box styles would cause a lot of work for a team to update to, they'd still be able to pull in the new button styles and only update their box styles when they had the time to. If we weren't using Lerna I'd still want to set the repo up this way, Lerna just makes it easier.

Also re working on this in dev (in case that's what you were curious about), Lerna handles symlinking and when we run the bootstrap script it will look at the changes logged in git and ask us what we want to bump the version number too automatically. So having individually versioned should be fine. I'm testing this in a another repo before blowing everything up here so will see how it goes!

@muan
Copy link
Contributor

muan commented Jun 2, 2017

Not without spending some time searching around, no. But it doesn't matter to me that those projects aren't individually versioning their packages, it's a supported feature of Lerna and seems like the right thing to do for us, and it maintains our current versioning system.

Sorry, I'm not doubting if we should do this, but asking what does this set up look like in real life. We should 100% version them separately.

@broccolini
Copy link
Member Author

Updates and notes from conversations with @muan and @jonrohan:

  • We don't think we'll be able to point to a branch in github/github when testing changes to Primer because of there is not git url mapped to the packages on npm, but we can publish a tagged pre-release. This will be fine once we have more components updated and in Primer but it might feel a little painful at first, and I think there's other things we can do to improve this new workflow (such as automating component testing or being able to spit out a list of all component variations).
  • We're unsure why Greenkeeper stopped working, may be to do with the authentication changes a while back, will ask @jonrohan for help with setting that up again.
  • Did we use ava for testing previously? If so, do we need to do anything re setup with the new monorepo? cc @jonrohan
  • I've updated the pr to reflect pre-ship and post-ship tasks so it's clear what we need to do for shipping this.

 - primer-alerts@1.1.3-mono.0
 - primer-avatars@0.4.7-mono.0
 - primer-base@1.0.1-mono.0
 - primer-blankslate@0.3.6-mono.0
 - primer-box@2.1.3-mono.0
 - primer-breadcrumb@0.1.2-mono.0
 - primer-buttons@2.0.1-mono.0
 - primer-cards@0.1.3-mono.0
 - primer-core@3.0.1-mono.0
 - primer-forms@1.0.7-mono.0
 - primer-labels@1.1.1-mono.0
 - primer-layout@0.3.3-mono.0
 - primer-markdown@3.3.8-mono.0
 - primer-marketing-type@0.2.1-mono.0
 - primer-marketing-utilities@0.1.5-mono.0
 - primer-marketing@3.0.1-mono.0
 - primer-navigation@1.0.1-mono.0
 - primer-page-headers@0.1.2-mono.0
 - primer-page-sections@0.1.2-mono.0
 - primer-product@3.0.1-mono.0
 - primer-support@4.0.1-mono.0
 - primer-table-object@1.0.4-mono.0
 - primer-tables@0.1.3-mono.0
 - primer-tooltips@0.5.5-mono.0
 - primer-truncate@0.3.3-mono.0
 - primer-utilities@4.2.5-mono.0
I think this is only in this package because it's the only that needed
updating when I ran lerna boostrap
 - primer-css@7.0.0-3
 - primer-marketing@4.0.0-3
 - primer-css@7.0.0-4
 - primer-marketing@4.0.0-4
 - primer-css@7.0.0-5
 - primer-marketing@4.0.0-5
 - primer-css@7.0.0-6
@broccolini
Copy link
Member Author

@shawnbot Here's the error I'm getting:

screenshot 2017-06-22 18 03 43

This was after I updated the marketing package to include the import for primer-marketing-utilities and also added that as a dependency. After updating packages, I ran lerna bootstrap to symlink the new dependency. Then ran lerna publish and got the above errors.

I thought maybe it was just having an issue with previous versions of how the module were setup, so I ran lerna clean to clean out the node modules, and then re-ran lerna bootstrap to symlink all the packages again. This time lockfiles were added to each package which I removed as previously this seemed to cause an issue with Lerna, but I kept everything else the same. After running lerna publish again now I get the same error above on other packages:
screenshot 2017-06-23 12 03 09

I manually removed the node modules and tried this all over again, to no avail. I've tried manually installing packages and I tried this with and without lockfiles. I started to look through issues in the Lerna repo, and found that lerna isn't compatible with npm @5, which I am on. So that may be why I'm getting a bunch of issues 😑 .

cc @jonrohan

@shawnbot
Copy link
Contributor

Here's a quick update, which I also posted to Slack. In case it matters, I'm running Node v8.1.2 and npm@5.0.3. I ran npm i -g lerna@2.0.0-rc.2 to globally install the same version of Lerna as the repo uses.

First, I looked at the lerna publish flags and saw that, even though Lerna doesn't support dry runs yet, there are CLI flags to disable the git and npm interactions. I bumped up the log level to debug, told it to skip npm and git, and (just to be safe) set the npm dist-tag to experimental:

lerna publish --loglevel debug --skip-npm --skip-git --npm-tag experimental

This prompted me to confirm the new versions for both the primer-css and primer-marketing packages. I chose 7.0.0 for primer-css and 4.0.0 for primer-marketing, which just removes the prerelease identifiers from both.

This completed without any errors and produced the following diff:

diff --git a/packages/primer-css/package.json b/packages/primer-css/package.json
index 4cd021d..77eb2ef 100644
--- a/packages/primer-css/package.json
+++ b/packages/primer-css/package.json
@@ -2 +2 @@
-  "version": "7.0.0-6",
+  "version": "7.0.0",
@@ -25 +25 @@
-    "primer-marketing": "^4.0.0-5",
+    "primer-marketing": "^4.0.0",
diff --git a/packages/primer-marketing/package.json b/packages/primer-marketing/package.json
index 234ed48..a66041a 100644
--- a/packages/primer-marketing/package.json
+++ b/packages/primer-marketing/package.json
@@ -2 +2 @@
-  "version": "4.0.0-5",
+  "version": "4.0.0",

There are also new (to git) package-lock.json files in each of the package directories, which I think is an npm@5 side effect. According to this Stack Overflow answer, these files are intended to be checked in.

While surfing around for more info, I discovered a couple of things:

  1. There's a Lerna 2.0.0 RC5 in their releases, and a bunch of bug fixes since RC2.
  2. Lerna supports a "hoist" feature that may help us here, at least in the sense that it'll make lerna bootstrap a lot quicker. (Bootstrapping took at least 5 minutes on my machine! 😬 ) I don't think that it affects publishing, though.
  3. This comment suggests declaring npm@4 as a dev dependency and setting Lerna's npmClient to the locally installed npm binary. I did that in 6ee8507, re-ran the above command, and got the same result minus the package-lock.json files.

And now, off to the screen hero!

 - primer-alerts@1.1.3
 - primer-avatars@0.4.7
 - primer-base@1.1.0
 - primer-blankslate@0.3.6
 - primer-box@2.1.3
 - primer-breadcrumb@0.1.2
 - primer-buttons@2.0.1
 - primer-cards@0.1.3
 - primer-core@4.0.0
 - primer-css@7.0.0-rc.0
 - primer-forms@1.0.8
 - primer-labels@1.1.1
 - primer-layout@1.0.0
 - primer-markdown@3.3.8
 - primer-marketing-support@0.5.1
 - primer-marketing-type@0.2.1
 - primer-marketing-utilities@0.1.5
 - primer-marketing@4.0.0
 - primer-navigation@1.0.1
 - primer-page-headers@0.1.2
 - primer-page-sections@0.1.2
 - primer-product@4.0.0
 - primer-support@4.0.2
 - primer-table-object@1.0.4
 - primer-tables@0.1.3
 - primer-tooltips@0.5.5
 - primer-truncate@0.3.3
 - primer-utilities@4.3.0
 - primer-alerts@1.1.4
 - primer-avatars@0.4.8
 - primer-base@1.1.1
 - primer-blankslate@0.3.7
 - primer-box@2.1.4
 - primer-breadcrumb@0.1.3
 - primer-buttons@2.0.2
 - primer-cards@0.1.4
 - primer-core@4.0.1
 - primer-css@7.0.0-rc.1
 - primer-forms@1.0.9
 - primer-labels@1.1.2
 - primer-layout@1.0.1
 - primer-markdown@3.3.9
 - primer-marketing-support@0.5.2
 - primer-marketing-type@0.2.2
 - primer-marketing-utilities@0.1.6
 - primer-marketing@4.0.1
 - primer-navigation@1.0.2
 - primer-page-headers@0.1.3
 - primer-page-sections@0.1.3
 - primer-product@4.0.1
 - primer-support@4.0.3
 - primer-table-object@1.0.5
 - primer-tables@0.1.4
 - primer-tooltips@0.5.6
 - primer-truncate@0.3.4
 - primer-utilities@4.3.1
 - primer-css@7.0.0-rc.2
Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

@broccolini and I worked out the build issues in primer-css, and we figured out how Lerna works (kind of). This works for now, and we've published RC2 to npm. Onwards and upwards!

@hzoo
Copy link

hzoo commented Jun 24, 2017

I guess this is rather late but I can answer questions y'all might have about Lerna (I maintain Babel and Lerna somewhat). Not sure how well it works with npm 5 and hoisting since never looked into it, but does seem like everyone did their research pretty well 😄 (lerna slack: https://slack.lernajs.io/, babel: slack.babeljs.io)

FYI I saw this via github/opensource.guide#404 so not just randomly commenting on issues

@broccolini
Copy link
Member Author

Thanks @hzoo! I had joined the lerna slack but @shawnbot ended up helping me solve the issues we were having. There's some follow up work to do on our side and there's some feedback we'll pass on to the you and the Lerna maintainers if issues haven't been opened already.

I don't think the problems we hit were really npm@5 issues, however it didn't seem to like the lockfile. Either way, since there are so many bug reports for npm@5 at the moment, it seems sensible to stick with 4 for now.

Also, you're more than welcome to randomly comment on our Primer pr's 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants