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] Update monorepo #3424

Merged
merged 7 commits into from
Apr 29, 2024
Merged

[core] Update monorepo #3424

merged 7 commits into from
Apr 29, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 19, 2024

@oliviertassinari oliviertassinari added the dependencies Update of dependencies label Apr 19, 2024
@oliviertassinari oliviertassinari force-pushed the upgrade-monorepo branch 2 times, most recently from 2a8652d to e606ad9 Compare April 19, 2024 22:29
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 19, 2024

It fails because @mui/docs wasn't released with a version that is compatible with @mui/monorepo yet.

It works for Base UI mui/base-ui#333 and MUI X mui/mui-x#12861 because we don't use @mui/docs there yet.

I still struggle to connect the dots to why npm dependency > git dependency for docs infra.

  • dependencies: it seems that pnpm can solve this, by not using the workspace syntax, and by not publishing to npm (to avoid silent bugs).
  • build step: not needed, we can configure Next.js in the dependent, the same way as in the source
  • code isolation: having one or multiple units of code doesn't seem to make a difference. What seems to matter is no leaking abstractions. e.g. micro-services aren't solving the problem.
  • else?

package.json Outdated Show resolved Hide resolved
@Janpot
Copy link
Member

Janpot commented Apr 20, 2024

I still struggle to connect the dots to why npm dependency > git dependency for docs infra.

The problems you encounter are caused by moving @mui/docs to the published variant before fully removing the monorepo dependency. Now we ended up with two code dependencies that have some shared assumptions about which version is used, but which are imported through two different release cycles.

My initial preference was to keep aliasing it from the monorepo dependency until we could fully replace, but the team is eager to move forward with publishing.

Similar problems will appear when introducing a different release process for internal and external packages. Unfortunately so far I haven't been able to convince the team about this yet. Will keep trying to find the right argument.

In general it's simple: a hybrid of monorepo+aliasing and published packages will cause more problems than either of those on their own. I am going to be clear about the following, if we're moving forward with published npm packages we're going to have a worse system until:

  1. we remove the monorepo dependency completely
  2. we adopt the same release cycle for internal packages and external ones

I also agree that we should have tried a git dependency first (still the format of npm packages, but unpublished), at least as a stopgap solution.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 20, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 20, 2024

Something is off with the latest version mui/mui-x#12861 (comment)

oliviertassinari added a commit to mui/mui-x that referenced this pull request Apr 20, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 20, 2024
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Revoking my review, I didn't look at the CI

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 21, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 24, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 26, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2024
@Janpot Janpot enabled auto-merge (squash) April 29, 2024 14:46
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

monorepo is up to date now, so I just kept the change to the config.
Added a few more comments around @mui/docs as well

@Janpot Janpot merged commit 3d4dbfd into mui:master Apr 29, 2024
12 checks passed
@oliviertassinari oliviertassinari deleted the upgrade-monorepo branch April 29, 2024 16:09
@oliviertassinari
Copy link
Member Author

It looks great, thanks

joakimtveter pushed a commit to joakimtveter/mui-x that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants