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.

Sign up for GitHub

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

Jump to bottom

[core] Explicitly define package dependencies #38859

Merged
merged 2 commits into from
Sep 7, 2023
Merged

[core] Explicitly define package dependencies #38859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10bac2a
[core] Explicitly define package dependencies
michaldudak Sep 7, 2023
33343a0
Reduce typecheck concurrency
michaldudak Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions benchmark/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,19 @@
"dependencies": {
"@chakra-ui/system": "^2.6.0",
"@emotion/react": "^11.11.1",
"@emotion/server": "^11.11.0",
"@emotion/styled": "^11.11.0",
"@mdx-js/react": "^2.3.0",
"@mui/material": "^5.14.8",
"@mui/styles": "^5.14.7",
"@mui/system": "^5.14.8",
"@styled-system/css": "^5.1.5",
"benchmark": "^2.1.4",
"docs": "^5.0.0",
"express": "^4.18.2",
"fs-extra": "^11.1.1",
"jss": "^10.10.0",
"playwright": "^1.37.1",
"prop-types": "^15.8.1",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-is": "^18.2.0",
Expand All @@ -29,8 +34,8 @@
"redux": "^4.2.1",
"serve-handler": "^6.1.5",
"styled-components": "^5.3.11",
"styled-system": "^5.1.5",
"theme-ui": "^0.16.1",
"webpack": "^5.88.2",
"webpack-cli": "^4.10.0"
"webpack": "^5.88.2"
}
}
30 changes: 22 additions & 8 deletions docs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"dependencies": {
"@babel/core": "^7.22.10",
"@babel/plugin-transform-object-assign": "^7.22.5",
"@babel/runtime": "^7.22.10",
"@babel/runtime-corejs2": "^7.22.10",
"@docsearch/react": "^3.5.2",
"@emotion/cache": "^11.11.0",
Expand All @@ -44,6 +45,7 @@
"@mui/styles": "^5.14.7",
"@mui/system": "^5.14.8",
"@mui/types": "^7.2.4",
"@mui/utils": "^5.14.8",
"@mui/x-charts": "^6.0.0-alpha.9",
"@mui/x-data-grid": "6.12.1",
"@mui/x-data-grid-generator": "6.12.1",
Expand All @@ -52,8 +54,9 @@
"@mui/x-date-pickers": "6.12.1",
"@mui/x-date-pickers-pro": "6.12.1",
"@mui/x-license-pro": "6.10.2",
"@mui/x-tree-view": "6.0.0-alpha.1",
"@popperjs/core": "^2.11.8",
"@react-spring/web": "^9.7.3",
"@trendmicro/react-interpolate": "^0.5.5",
"@types/autosuggest-highlight": "^3.2.0",
"@types/css-mediaquery": "^0.1.1",
"@types/json2mq": "^0.2.0",
Expand All @@ -77,8 +80,6 @@
"css-mediaquery": "^0.1.2",
"date-fns": "^2.30.0",
"date-fns-jalali": "^2.21.3-1",
"exceljs": "^4.3.0",
"express": "^4.18.2",
"feed": "^4.2.2",
"fg-loadcss": "^3.1.0",
"final-form": "^4.20.10",
Expand Down Expand Up @@ -109,26 +110,39 @@
"react-runner": "^1.0.3",
"react-simple-code-editor": "^0.13.1",
"react-swipeable-views": "^0.14.0",
"react-swipeable-views-utils": "^0.14.0",
"react-transition-group": "^4.4.5",
"react-virtuoso": "^4.5.0",
"react-window": "^1.8.9",
"recharts": "2.7.3",
"rimraf": "^5.0.1",
"styled-components": "^5.3.11",
"stylis": "^4.2.0",
"stylis": "4.2.0",
"stylis-plugin-rtl": "^2.1.1",
"stylis-plugin-rtl-sc": "npm:stylis-plugin-rtl@^1.1.0",
"use-count-up": "^3.0.1",
"webfontloader": "^1.6.28",
"webpack-bundle-analyzer": "^4.9.1"
},
"devDependencies": {
"@babel/plugin-transform-react-constant-elements": "^7.22.5",
"@babel/preset-typescript": "^7.22.5",
"@types/marked": "^5.0.1",
"@types/recharts": "^2.0.0",
"@mui-internal/docs-utilities": "^1.0.0",
"@types/chai": "^4.3.5",
"@types/node": "^18.17.6",
"@types/prop-types": "^15.7.5",
"@types/react": "^18.2.21",
"@types/react-dom": "18.2.7",
Copy link
Member

Choose a reason for hiding this comment

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

yarn docs:dev now returns a new warning:

Screenshot 2023-09-11 at 02 21 26

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

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 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)

Copy link
Member

@Janpot Janpot Sep 11, 2023 •
edited

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Janpot Janpot Sep 13, 2023 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"@types/react-transition-group": "^4.4.6",
"@types/stylis": "^4.2.0",
"chai": "^4.3.7",
"cross-fetch": "^4.0.0",
"gm": "^1.25.0",
"tailwindcss": "^3.3.3"
"marked": "^5.1.2",
"playwright": "^1.36.2",
"prettier": "^2.8.8",
"tailwindcss": "^3.3.3",
"test": "5.0.0",
"typescript-to-proptypes": "^5.0.0",
"yargs": "^17.7.2"
}
}
55 changes: 12 additions & 43 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"test:unit": "cross-env NODE_ENV=test mocha 'packages/**/*.test.{js,ts,tsx}' 'docs/**/*.test.{js,ts,tsx}' 'test/utils/**/*.test.{js,ts,tsx}'",
"test:argos": "node ./scripts/pushArgos.mjs",
"typescript": "lerna run --no-bail --parallel typescript",
"typescript:ci": "lerna run --concurrency 7 --no-bail --no-sort typescript",
"typescript:ci": "lerna run --concurrency 5 --no-bail --no-sort typescript",
"validate-declarations": "ts-node --esm scripts/validateTypescriptDeclarations.mts",
"generate-codeowners": "node scripts/generateCodeowners.mjs"
},
Expand All @@ -88,31 +88,24 @@
"@babel/preset-env": "^7.22.10",
"@babel/preset-react": "^7.22.5",
"@babel/register": "^7.22.5",
"@emotion/react": "^11.11.1",
"@emotion/styled": "^11.11.0",
"@googleapis/sheets": "^5.0.4",
"@mnajdova/enzyme-adapter-react-18": "^0.2.0",
"@mui-internal/api-docs-builder": "^1.0.0",
"@mui-internal/docs-utilities": "^1.0.0",
"@mui/joy": "5.0.0-beta.5",
"@mui/material": "^5.14.8",
"@mui/utils": "^5.14.8",
"@octokit/rest": "^20.0.1",
"@playwright/test": "1.37.1",
"@rollup/plugin-replace": "^5.0.2",
"@slack/bolt": "^3.13.3",
"@testing-library/dom": "^9.3.1",
"@testing-library/react": "^14.0.0",
"@testing-library/user-event": "^14.4.3",
"@types/chai": "^4.3.5",
"@types/chai-dom": "^1.11.0",
"@slack/web-api": "^6.9.0",
"@types/enzyme": "^3.10.13",
"@types/format-util": "^1.0.2",
"@types/fs-extra": "^11.0.1",
"@types/lodash": "^4.14.197",
"@types/mocha": "^10.0.1",
"@types/node": "^18.17.14",
"@types/prettier": "^2.7.3",
"@types/react": "^18.2.21",
"@types/react-is": "^18.2.1",
"@types/react-test-renderer": "^18.0.0",
"@types/sinon": "^10.0.16",
"@types/stylis": "^4.2.0",
"@types/yargs": "^17.0.24",
"@typescript-eslint/eslint-plugin": "^6.4.1",
"@typescript-eslint/parser": "^6.4.1",
Expand All @@ -122,40 +115,33 @@
"babel-plugin-module-resolver": "^5.0.0",
"babel-plugin-optimize-clsx": "^2.6.2",
"babel-plugin-react-remove-properties": "^0.3.0",
"babel-plugin-tester": "^11.0.4",
"babel-plugin-transform-react-remove-prop-types": "^0.4.24",
"chai": "^4.3.8",
"chai-dom": "^1.11.0",
"chalk": "^5.3.0",
"compression-webpack-plugin": "^10.0.0",
"concurrently": "^8.2.1",
"confusing-browser-globals": "^1.0.11",
"core-js": "^2.6.11",
"cpy-cli": "^5.0.0",
"cross-env": "^7.0.3",
"danger": "^11.2.8",
"dom-accessibility-api": "^0.6.1",
"dtslint": "^4.2.1",
"enzyme": "^3.11.0",
"eslint": "^8.48.0",
"eslint-config-airbnb": "^19.0.4",
"eslint-config-airbnb-base": "^15.0.0",
"eslint-config-airbnb-typescript": "^17.1.0",
"eslint-config-prettier": "^8.10.0",
"eslint-import-resolver-webpack": "^0.13.7",
"eslint-plugin-babel": "^5.3.1",
"eslint-plugin-filenames": "^1.3.2",
"eslint-plugin-import": "^2.28.1",
"eslint-plugin-jsx-a11y": "^6.7.1",
"eslint-plugin-material-ui": "^5.0.0",
"eslint-plugin-mocha": "^10.1.0",
"eslint-plugin-react": "^7.32.2",
"eslint-plugin-react-hooks": "^4.6.0",
"fast-glob": "^3.3.1",
"format-util": "^1.0.5",
"fs-extra": "^11.1.1",
"globby": "^13.2.2",
"google-auth-library": "^9.0.0",
"html-webpack-plugin": "^5.5.3",
"jsdom": "^22.1.0",
"karma": "^6.4.2",
"karma-browserstack-launcher": "~1.6.0",
"karma-chrome-launcher": "^3.2.0",
Expand All @@ -164,45 +150,28 @@
"karma-mocha": "^2.0.1",
"karma-sourcemap-loader": "^0.4.0",
"karma-webpack": "^5.0.0",
"lerna": "^7.1.5",
"lerna": "^7.2.0",
"lodash": "^4.17.21",
"markdownlint-cli2": "^0.9.2",
"mocha": "^10.2.0",
"nx": "^16.7.4",
"nyc": "^15.1.0",
"piscina": "^4.1.0",
"postcss-styled-syntax": "^0.4.0",
"prettier": "^2.8.8",
"pretty-quick": "^3.1.3",
"process": "^0.11.10",
"prop-types": "^15.8.1",
"raw-loader": "4.0.2",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-router-dom": "^6.15.0",
"react-test-renderer": "^18.2.0",
"remark": "^13.0.0",
"rimraf": "^5.0.1",
"rollup": "^3.28.1",
"rollup-plugin-babel": "^4.4.0",
"rollup-plugin-commonjs": "^10.1.0",
"rollup-plugin-node-globals": "^1.4.0",
"rollup-plugin-node-resolve": "^5.2.0",
"rollup-plugin-terser": "^7.0.2",
"serve": "^14.2.1",
"sinon": "^15.2.0",
"stylelint": "^15.10.3",
"stylelint-config-standard": "^34.0.0",
"stylelint-processor-styled-components": "^1.10.0",
"stylis": "^4.2.0",
"stylis-plugin-rtl": "^2.1.1",
"stylis-plugin-rtl-sc": "npm:stylis-plugin-rtl@^1.1.0",
"terser-webpack-plugin": "^5.3.9",
"thenify": "3.3.1",
"ts-node": "^10.9.1",
"tslint": "5.14.0",
"typescript": "^5.1.6",
"unist-util-visit": "^2.0.3",
"util": "^0.12.5",
"typescript-to-proptypes": "^5.0.0",
"webpack": "^5.88.2",
"webpack-bundle-analyzer": "^4.9.1",
"webpack-cli": "^4.10.0",
Expand Down
16 changes: 15 additions & 1 deletion packages/api-docs-builder/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,31 @@
},
"dependencies": {
"@babel/core": "^7.22.10",
"@babel/preset-typescript": "^7.22.5",
"@babel/traverse": "^7.22.10",
"@mui-internal/docs-utilities": "^1.0.0",
"@mui/markdown": "^5.0.0",
"@mui/utils": "^5.14.8",
"ast-types": "^0.14.2",
"docs": "^5.0.0",
"doctrine": "^3.0.0",
"fast-glob": "^3.3.1",
"fs-extra": "^11.1.1",
"lodash": "^4.17.21",
"prettier": "^2.8.8",
"react-docgen": "^5.4.3",
"recast": "^0.23.4",
"typescript-to-proptypes": "^5.0.0",
"remark": "^13.0.0",
"unist-util-visit": "^2.0.3",
"yargs": "^17.7.2"
},
"devDependencies": {
"@types/babel__core": "^7.20.1",
"@types/babel__traverse": "^7.20.1",
"@types/chai": "^4.3.5",
"@types/sinon": "^10.0.16",
"@types/doctrine": "^0.0.5",
"@types/mdast": "4.0.0",
"@types/mocha": "^10.0.1",
"@types/node": "^18.17.14",
"chai": "^4.3.8",
Expand Down
3 changes: 2 additions & 1 deletion packages/eslint-plugin-material-ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

@Janpot Janpot Sep 14, 2023 •
edited

Choose a reason for hiding this comment

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

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

},
"scripts": {
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/eslint-plugin-material-ui/**/*.test.js'"
Expand Down
5 changes: 5 additions & 0 deletions packages/markdown/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,13 @@
"./prism": "./prism.js"
},
"dependencies": {
"@babel/runtime": "^7.22.10",
"lodash": "^4.17.21",
"marked": "^5.1.2",
"prismjs": "^1.29.0"
},
"devDependencies": {
"@types/chai": "^4.3.5",
"chai": "^4.3.7"
}
}
38 changes: 27 additions & 11 deletions packages/mui-base/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,33 @@
"typescript": "tslint -p tsconfig.json \"{src,test}/**/*.{spec,d}.{ts,tsx}\" && tsc -p tsconfig.json",
"typescript:module-augmentation": "node scripts/testModuleAugmentation.js"
},
"dependencies": {
"@babel/runtime": "^7.22.10",
"@floating-ui/react-dom": "^2.0.2",
"@mui/types": "^7.2.4",
"@mui/utils": "^5.14.8",
"@popperjs/core": "^2.11.8",
"clsx": "^2.0.0",
"prop-types": "^15.8.1"
},
"devDependencies": {
"@mui/base": "5.0.0-beta.14",
"@mui/material": "^5.14.8",
"@testing-library/react": "^14.0.0",
"@testing-library/user-event": "^14.4.3",
"@types/chai": "^4.3.5",
"@types/prop-types": "^15.7.5",
"@types/react": "18.2.21",
"@types/react-dom": "18.2.7",
"@types/sinon": "^10.0.16",
"chai": "^4.3.7",
"fast-glob": "^3.3.1",
"lodash": "^4.17.21",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"sinon": "^15.2.0",
"test": "^5.0.0"
},
"peerDependencies": {
"@types/react": "^17.0.0 || ^18.0.0",
"react": "^17.0.0 || ^18.0.0",
Expand All @@ -50,17 +77,6 @@
"optional": true
}
},
"dependencies": {
"@babel/runtime": "^7.22.10",
"@emotion/is-prop-valid": "^1.2.1",
"@floating-ui/react-dom": "^2.0.2",
"@mui/types": "^7.2.4",
"@mui/utils": "^5.14.8",
"@popperjs/core": "^2.11.8",
"clsx": "^2.0.0",
"prop-types": "^15.8.1",
"react-is": "^18.2.0"
},
"sideEffects": false,
"publishConfig": {
"access": "public"
Expand Down
4 changes: 3 additions & 1 deletion packages/mui-codemod/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
"yargs": "^17.7.2"
},
"devDependencies": {
"@types/jscodeshift": "0.11.5"
"@types/chai": "^4.3.5",
"@types/jscodeshift": "0.11.5",
"chai": "^4.3.7"
},
"sideEffects": false,
"publishConfig": {
Expand Down