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

[core] Use Lerna to publish #23793

Merged
merged 2 commits into from
Dec 3, 2020
Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 30, 2020

Closes #23785. The usage of Lerna should solve the pain around managing the version between the different packages. I have been doing it by hand so far. However, it's error-prone, doesn't scale with the number of packages, and time-consuming.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Nov 30, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 30, 2020

No bundle size changes

Generated by 🚫 dangerJS against 51cbda0

@oliviertassinari oliviertassinari force-pushed the lerna branch 2 times, most recently from 9225c33 to 52ad2e8 Compare November 30, 2020 23:47
@oliviertassinari oliviertassinari marked this pull request as ready for review December 1, 2020 00:10
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

What are all the changes to the tsconfigs for?

Comment on lines +10 to +11
"release": "yarn build:release && lerna publish --no-changelog --dist-tag next --contents build",
"publish:dry-run": "lerna publish --no-changelog --dist-tag next --contents build --no-git-tag-version --no-push --registry=\"http://localhost:4873/\"",
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some docs to the release process while we're at it? It's unclear how this should be used.

Copy link
Member Author

@oliviertassinari oliviertassinari Dec 1, 2020

Choose a reason for hiding this comment

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

Sure, I would be happy too. The objective of the dry-run command was to test that the release process works end-to-end. I have used https://github.com/verdaccio/verdaccio.

Copy link
Member Author

@oliviertassinari oliviertassinari Dec 1, 2020

Choose a reason for hiding this comment

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

Could you add some docs to the release process while we're at it?

Done https://trello.com/c/H7Tz29ft/2763-release-steps. It's almost identical to the release steps of Material-UI X.

Copy link
Member

Choose a reason for hiding this comment

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

I meant documenting as in adding a section to CONTRIBUTING.md so that we can always keep it in sync.

Copy link
Member Author

@oliviertassinari oliviertassinari Dec 2, 2020

Choose a reason for hiding this comment

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

I think that moving it to git would have the opposite effect, it will make updating the instructions more painful (opening a PR vs. doing the edits directly), so more likely to go out of sync. I think it's why the GitHub Wiki is using a different edit workflow tradeoff. Many of the steps are not directly related to the code. But happy to give it a try if you think it's important.

Copy link
Member

@eps1lon eps1lon Dec 2, 2020

Choose a reason for hiding this comment

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

(opening a PR vs. doing the edits directly),

I suspected that you're less and less interested in having your work reviewed. But you seem to be doing fine with just committing if you don't want people to review it. Why would that not work here?

Copy link
Member

Choose a reason for hiding this comment

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

it will make updating the instructions more painful

Then we need to work on the pain points when opening a PR. What are frequent problems you run into?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that keeping in sync and helping with reviews are two different topics. I thought the concern was with keeping in sync with the code.

I had assumed that we wouldn't need to review changes to the release workflow instructions, that they are meant to be an unbiased list of steps to follow, that if there is a mistake in it, anybody would go and update the instructions to fix them.

I had also assumed that reviewing the release workflow would require a different discussion. I'm still happy to do it if you think it's important. React has it documented in git: https://github.com/facebook/react/blob/master/scripts/release/README.md.

Copy link
Member

Choose a reason for hiding this comment

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

I had assumed that we wouldn't need to review changes to the release workflow instructions, that they are meant to be an unbiased list of steps to follow, that if there is a mistake in it, anybody would go and update the instructions to fix them.

Same exact reasons why we keep any documentation checked-in. The release workflow isn't something special that needs to be kept in a separate, inaccessible place. At least I can't see a reasons from what you've told. Maybe there are other factors you're not sharing but I can't address those.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved the content of the Trello card to a readme.

@@ -1,5 +1,4 @@
{
"packages": ["packages/*", "docs", "framer"],
Copy link
Member Author

Choose a reason for hiding this comment

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

Has no effect, useWorkspaces override it.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 1, 2020

What are all the changes to the tsconfigs for?

@eps1lon There are a couple of different changes in the tsconfigs. Some are purely about sorting the properties to be in the same order in all the files. I could move them to a "batch". I think that the only important change for the types is around tsconfig.build.composite.json. I can't get the cd packages/material-ui/ && yarn build command to work on my environment. Could you check that it works for you? It would help identify if it's an issue with my env or the dependencies. Thanks! The solution I have found in my env is to remove composite: true. I had to remove it to get the .ts -> .d.ts file generation work.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 1, 2020

Could you check that it works for you?

Actually, these reproduction steps aren't accurate. On the next branch, what I can reproduce the error with is this command:

yarn build:codesandbox

Capture d’écran 2020-12-01 à 17 05 04

Then, once it fails, I can have cd packages/material-ui/ && yarn build fail.

@eps1lon
Copy link
Member

eps1lon commented Dec 1, 2020

So can we handle this another time then? It sounds like the tsconfig changes are not blocking any step in the release process. Unless you're doing something difference, hence the request to add documentation.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 1, 2020

So can we handle this another time then?

@eps1lon During the last release, it has almost blocked me from releasing (v5.0.0-alpha.17). I had to find the trick (remove composite). Unfortunately it was breaking the CI, so I added it back in 86ec791 on next.

@eps1lon
Copy link
Member

eps1lon commented Dec 1, 2020

Could you please describe the steps that are breaking? First you said it was build:codesandbox but that's unrelated to releasing the packages.

If it's only cd ... && yarn * then use yarn workspace ... * instead.

@eps1lon
Copy link
Member

eps1lon commented Dec 1, 2020

Unfortunately it was breaking the CI, so I added it back in 86ec791 on next.

Well nobody can help you if you don't ask and silently commit to the repo. The CI is not for certain groups only. It helps everyone so please leverage it instead of skipping it. This can leave the repository in a broken state that nobody is aware of.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 1, 2020

Could you please describe the steps that are breaking?

A cleaner reproduction:

  1. Setup
git clone [email protected]:mui-org/material-ui.git foo
cd foo
yarn
  1. Build 1 ✅
cd packages/material-ui/ && yarn build
  1. Build the lab
cd ../packages/material-ui-lab/ && yarn build && cd ../../
  1. Build 2 ❌
cd packages/material-ui/ && yarn build

Capture d’écran 2020-12-01 à 21 21 05

The CI is not for certain groups only. It helps everyone so please leverage it instead of skipping it. This can leave the repository in a broken state that nobody is aware of.

That wasn't "clean", agree. Hopefully, the change was small enough to be easy to revert.

@eps1lon
Copy link
Member

eps1lon commented Dec 1, 2020

Perfect, it's already reproducible with just repeating yarn workspace @material-ui/core build. We clean the build folder but don't tell TypeScript that we changed the build state (i.e. delete tsconfig.build.tsbuildinfo. Will include fix in #23805

Update: Fixed by de34a4d (#23805)

@eps1lon eps1lon reopened this Dec 1, 2020
@eps1lon
Copy link
Member

eps1lon commented Dec 1, 2020

Should be rebased on next and the tsconfig changes removed.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 1, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 1, 2020
@oliviertassinari oliviertassinari force-pushed the lerna branch 2 times, most recently from 07b2df8 to 4b8373f Compare December 1, 2020 23:17
@oliviertassinari oliviertassinari force-pushed the lerna branch 2 times, most recently from 6c41c0d to 1b970ab Compare December 2, 2020 16:45
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 3, 2020
@oliviertassinari oliviertassinari merged commit 6cc38f9 into mui:next Dec 3, 2020
@oliviertassinari oliviertassinari deleted the lerna branch December 3, 2020 09:44
mbrookes pushed a commit to oliviertassinari/material-ui that referenced this pull request Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alpha version should have strict version range dependencies to other mui packages
3 participants