[core] Explicitly define package dependencies #38859

michaldudak · 2023-09-07T13:20:32Z

Extracted from #36287

In this PR, I listed all the dependencies in package.json files. This is not required by yarn (but it's a good practice anyway), but pnpm needs it.

Additionally, I standardized the layout of package.json files (order of fields).

mui-bot · 2023-09-07T13:29:30Z

Netlify deploy preview

https://deploy-preview-38859--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 33343a0

Janpot

Not reading every line, but 👍 on the methodology.

michaldudak · 2023-09-07T14:04:03Z

For the record, I used depcheck to analyze what's missing and what's redundant. It required some manual adjustments afterward, but the passing build and tests should be a good indication that it works.

oliviertassinari

Are we sure about these changes? I see a lot more installation warnings (yarn):

Before

After

(truncated, can't take a screenshot with everything)

Can we fix all these?

For example, why is @types/react not set only once in the root package.json workspace?

oliviertassinari · 2023-09-11T00:24:39Z

docs/package.json

+    "@types/node": "^18.17.6",
+    "@types/prop-types": "^15.7.5",
+    "@types/react": "^18.2.21",
+    "@types/react-dom": "18.2.7",


yarn docs:dev now returns a new warning:

How about we move all the @types dependencies of docs to be devDependencies?

I see a lot more installation warnings (yarn)

We've got more dependencies now, so it's expected that the same errors will appear in more places, where the dependencies are defined.
However, some of them look like false positives (react as peer dependency). I'll see what I can do about them.

Can we fix all these?

pnpm doesn't report these, so we should be good after we switch to it.

why is @types/react not set only once in the root package.json workspace

Because it's used by individual packages, not just the scripts at the workspace root.

How about we move all the @types dependencies of docs to be devDependencies?

Yup, types should be in dev dependencies. I'll fix it (#38914)

However, some of them look like false positives (react as peer dependency). I'll see what I can do about them.

👍 Many of them are top-level packages that define react as a peer dependency, but we don't have react as a workspace root dependency, so there is no strong guarantee about which version of react ends up adjacent to them in the node_modules folder. Just installing react and react-dom as a dev dependency in the root should solve a lot of these false positives.

The others that complain about incompatible react version are more problematic. These may work now, but as the authors don't guarantee compatibility with React 18, technically there may be breaking changes introduced for these packages on minor React versions.

edit: I opened a quick notion page to document our strategy towards getting rid of all these warnings. But perhaps a GitHub issue makes more sense?

pnpm doesn't report these, so we should be good after we switch to it.

Ah, ok, great.

Because it's used by individual packages, not just the scripts at the workspace root.

OK, but it feels better DX to have the common dev dependencies defined only once at the root of the workspace and to only define the ones that can't upgrade just yet to stay on an older version in the sub package.json (it's less duplication). Is this a pnpm limitation that yarn doesn't have?

Yes, pnpm requires packages to be defined where they are used.

Yes, this should work, but why would you want to treat @types/react differently than the rest of the dependencies? Versions are handled by renovate anyway, so updating in individual packages is as easy as in workspace root.

but why would you want to treat @types/react differently than the rest of the dependencies?

I agree, my strictly personal preference would also be to be explicit in declaring all dependencies per workspace. It makes the workspaces more self-contained. But I don't think in our current situation there is much practical benefit of one method over the other so either way is fine for me.

forcing a default of sharing the same version while keeping flexibility

For reference, pnpm has an RFC that should help with enforcing a single version across all workspaces. There also seems to be an intent to implement.

edit: @michaldudak seeing this RFC reminds me that it probably makes sense to take pnpm-workspaces.yaml into account for calculating the pnpm cache key in CI.

I agree, my strictly personal preference would also be to be explicit in declaring all dependencies per workspace. It makes the workspaces more self-contained.

What's the value of having the dev dependencies that are used in, say 80% of the codebase, to be defined in each workspace (where used)?

I'm blind to the value, the closest I can understand would be allowing us to move from yarn to pnpm to have faster npm install, otherwise I don't understand, I'm missing something. I fear for the problems it creates, e.g. #38920 (comment) or the RFC motivation.

I'm blind to the value

That's why I call it a strictly personal preference. I enjoy the idea that when I can copy a workspace folder to my desktop and run yarn inside of it and everything just works it's a sign of true encapsulation. Meaning the workspace function is to link packages together and nothing else and all other code sharing between packages happens through explicit dependencies.

But truely personal preference in my opinion, the monorepo has much more important issues than this one.

seeing this RFC reminds me that it probably makes sense to take pnpm-workspaces.yaml into account for calculating the pnpm cache key in CI.

@Janpot, for now, I followed the official guidelines about setting up the cache. When the catalogs are introduced, we'll have to adjust our setup anyway, so then we can add the workspace definition to the cache key.

Semigradsky · 2023-09-13T14:22:45Z

packages/mui-styled-engine/package.json

@@ -41,7 +41,17 @@
    "@babel/runtime": "^7.22.10",
    "@emotion/cache": "^11.11.0",
    "csstype": "^3.1.2",
-    "prop-types": "^15.8.1"
+    "prop-types": "^15.8.1",
+    "react": "^18.2.0"


Are you sure that we need react (!!!) in @mui/styled-engine dependencies?

cc @michaldudak

oliviertassinari · 2023-09-13T14:31:23Z

packages/mui-styled-engine-sc/package.json

-    "prop-types": "^15.8.1"
+    "csstype": "^3.1.2",
+    "prop-types": "^15.8.1",
+    "react": "^18.2.0"


Shoudn't be here

Suggested change

"react": "^18.2.0"

Yep, this should either have been defined on the workspace root, or it should have been a dev dependency

michaldudak · 2023-09-14T07:38:45Z

@Semigradsky, @oliviertassinari, fixed in #38971

Janpot

Just for good measure, I did go through it a bit more thoroughly. Left two minor comments.

Janpot · 2023-09-14T07:45:05Z

packages/eslint-plugin-material-ui/package.json

@@ -9,7 +9,8 @@
  },
  "devDependencies": {
    "@types/eslint": "^8.44.2",
-    "@typescript-eslint/parser": "^6.4.1"
+    "@typescript-eslint/parser": "^6.4.1",
+    "eslint": "^8.47.0"


Should this (technically) also be a peer dependency? It's an eslint plugin after all.

Janpot · 2023-09-14T08:41:58Z

packages/mui-styles/package.json

+  },
+  "peerDependencies": {
+    "@types/react": "^17.0.0 || ^18.0.0",
+    "react": "^17.0.0"


Are you aware of incompatibilities of this package with react 18? I think it's the only peer dependency conflict we experience on Toolpad. We import it for the docs. afaik our docs are on React 18, and this doesn't seem to be breaking them.

Toolpad docs don't directly depend on it, but we can't remove it yet: mui/toolpad#2684.

Yep, I've tried the same before 😄

Are you aware of incompatibilities of this package with react 18?

@mnajdova would likely know the best.

Are you aware of incompatibilities of this package with react 18?

One issue is GlobalStyle injection that is called twice in React 18 so not removed after unmounting.

michaldudak added the core Infrastructure work going on behind the scenes label Sep 7, 2023

michaldudak requested a review from Janpot September 7, 2023 13:21

[core] Explicitly define package dependencies

10bac2a

michaldudak force-pushed the explicit-dependencies branch from 0c4acfa to 10bac2a Compare September 7, 2023 13:25

Reduce typecheck concurrency

33343a0

michaldudak marked this pull request as ready for review September 7, 2023 13:53

michaldudak mentioned this pull request Sep 7, 2023

[core] Change package manager to pnpm #36287

Merged

Janpot approved these changes Sep 7, 2023

View reviewed changes

michaldudak merged commit 23724d1 into mui:master Sep 7, 2023
8 checks passed

michaldudak deleted the explicit-dependencies branch September 7, 2023 14:07

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023

[core] Explicitly define package dependencies (mui#38859)

d872d0d

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023

[core] Explicitly define package dependencies (mui#38859)

0093ce6

oliviertassinari reviewed Sep 11, 2023

View reviewed changes

oliviertassinari mentioned this pull request Sep 11, 2023

[core] Move types packages to docs' devDependencies #38914

Merged

xcode-it pushed a commit to xcode-it/material-ui that referenced this pull request Sep 11, 2023

[core] Explicitly define package dependencies (mui#38859)

3b48305

This was referenced Sep 12, 2023

[release] v5.14.9 #38920

Merged

[core] Remove package declaration from same package dependencies #38951

Merged

Semigradsky reviewed Sep 13, 2023

View reviewed changes

oliviertassinari reviewed Sep 13, 2023

View reviewed changes

joshkel mentioned this pull request Sep 13, 2023

[core] Missing peerDependencies for Yarn #38961

Closed

2 tasks

Janpot reviewed Sep 14, 2023

View reviewed changes

Methuselah96 mentioned this pull request Sep 17, 2023

[utils]: Missing dependency on @types/prop-types #39029

Closed

2 tasks

michaldudak mentioned this pull request Sep 18, 2023

[core] Move eslint to peer dependencies of eslint-plugin-material-ui #39033

Merged

ZeeshanTamboli mentioned this pull request Jan 29, 2024

[core] Remove unneeded @slack/web-api package #40840

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Explicitly define package dependencies #38859

[core] Explicitly define package dependencies #38859

michaldudak commented Sep 7, 2023 •

edited

Loading

mui-bot commented Sep 7, 2023 •

edited

Loading

Janpot left a comment

michaldudak commented Sep 7, 2023

oliviertassinari left a comment •

edited

Loading

oliviertassinari Sep 11, 2023

michaldudak Sep 11, 2023 •

edited

Loading

Janpot Sep 11, 2023 •

edited

Loading

oliviertassinari Sep 11, 2023 •

edited

Loading

michaldudak Sep 11, 2023

michaldudak Sep 13, 2023

Janpot Sep 13, 2023 •

edited

Loading

oliviertassinari Sep 13, 2023 •

edited

Loading

Janpot Sep 13, 2023

michaldudak Sep 14, 2023

Semigradsky Sep 13, 2023 •

edited

Loading

Semigradsky Sep 13, 2023

oliviertassinari Sep 13, 2023 •

edited

Loading

Janpot Sep 13, 2023 •

edited

Loading

michaldudak commented Sep 14, 2023

Janpot left a comment

Janpot Sep 14, 2023 •

edited

Loading

Janpot Sep 14, 2023 •

edited

Loading

oliviertassinari Sep 15, 2023 •

edited

Loading

Janpot Sep 15, 2023

michaldudak Sep 18, 2023

oliviertassinari Sep 18, 2023

[core] Explicitly define package dependencies #38859

[core] Explicitly define package dependencies #38859

Conversation

michaldudak commented Sep 7, 2023 • edited Loading

mui-bot commented Sep 7, 2023 • edited Loading

Netlify deploy preview

Bundle size report

Janpot left a comment

Choose a reason for hiding this comment

michaldudak commented Sep 7, 2023

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaldudak Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Janpot Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semigradsky Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Janpot Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

michaldudak commented Sep 14, 2023

Janpot left a comment

Choose a reason for hiding this comment

Janpot Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Janpot Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaldudak commented Sep 7, 2023 •

edited

Loading

mui-bot commented Sep 7, 2023 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

michaldudak Sep 11, 2023 •

edited

Loading

Janpot Sep 11, 2023 •

edited

Loading

oliviertassinari Sep 11, 2023 •

edited

Loading

Janpot Sep 13, 2023 •

edited

Loading

oliviertassinari Sep 13, 2023 •

edited

Loading

Semigradsky Sep 13, 2023 •

edited

Loading

oliviertassinari Sep 13, 2023 •

edited

Loading

Janpot Sep 13, 2023 •

edited

Loading

Janpot Sep 14, 2023 •

edited

Loading

Janpot Sep 14, 2023 •

edited

Loading

oliviertassinari Sep 15, 2023 •

edited

Loading