-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[docs-infra] Move BrandingProvider/brandingTheme/InfoCard to @mui/docs #41206
Conversation
Netlify deploy previewhttps://deploy-preview-41206--material-ui.netlify.app/ Bundle size report |
@@ -3,14 +3,23 @@ | |||
// Actual .ts source files are transpiled via babel | |||
"extends": "./tsconfig.json", | |||
"compilerOptions": { | |||
"paths": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaldudak I had to do some unholy stuff to deal with icon imports and the build:types
script. (also here). I can't reference a composite project for the icons package as that doesn't really exist. Any pointers on how you'd solve this?
To experience the problem, apply:
diff --git a/packages/mui-docs/modules.d.ts b/packages/mui-docs/modules.d.ts
deleted file mode 100644
index 25c6871850..0000000000
--- a/packages/mui-docs/modules.d.ts
+++ /dev/null
@@ -1,6 +0,0 @@
-declare module '@mui/icons-material/*' {
- import SvgIcon from '@mui/material/SvgIcon';
-
- const icon: typeof SvgIcon;
- export default icon;
-}
diff --git a/packages/mui-docs/tsconfig.build.json b/packages/mui-docs/tsconfig.build.json
index 8a527e26fb..24e2b1f591 100644
--- a/packages/mui-docs/tsconfig.build.json
+++ b/packages/mui-docs/tsconfig.build.json
@@ -3,12 +3,6 @@
// Actual .ts source files are transpiled via babel
"extends": "./tsconfig.json",
"compilerOptions": {
- "paths": {
- "@mui/material": ["./packages/mui-material/src/"],
- "@mui/material/*": ["./packages/mui-material/src/*"],
- "@mui/system": ["./packages/mui-system/src/"],
- "@mui/system/*": ["./packages/mui-system/src/*"]
- },
"composite": true,
"declaration": true,
"noEmit": false,
@@ -16,7 +10,7 @@
"outDir": "build",
"rootDir": "./src"
},
- "include": ["src/**/*.ts*", "./modules.d.ts"],
+ "include": ["src/**/*.ts*"],
"exclude": ["src/**/*.spec.ts*", "src/**/*.test.ts*"],
"references": [
{ "path": "../mui-material/tsconfig.build.json" },
You should get the following errors when running build:types
:
../mui-icons-material/lib/ArrowDropDownRounded.js:9:53 - error TS6059: File '/Users/janpotoms/Projects/material-ui/packages/mui-icons-material/lib/utils/createSvgIcon.js' is not under 'rootDir' '/Users/janpotoms/Projects/material-ui/packages/mui-docs/src'. 'rootDir' is expected to contain all source files.
9 var _createSvgIcon = _interopRequireDefault(require("./utils/createSvgIcon"));
~~~~~~~~~~~~~~~~~~~~~~~
../mui-icons-material/lib/ArrowDropDownRounded.js:9:53 - error TS6307: File '/Users/janpotoms/Projects/material-ui/packages/mui-icons-material/lib/utils/createSvgIcon.js' is not listed within the file list of project '/Users/janpotoms/Projects/material-ui/packages/mui-docs/tsconfig.build.json'. Projects must list all files or use an 'include' pattern.
9 var _createSvgIcon = _interopRequireDefault(require("./utils/createSvgIcon"));
~~~~~~~~~~~~~~~~~~~~~~~
src/branding/brandingTheme.ts:3:34 - error TS6059: File '/Users/janpotoms/Projects/material-ui/packages/mui-icons-material/lib/ArrowDropDownRounded.js' is not under 'rootDir' '/Users/janpotoms/Projects/material-ui/packages/mui-docs/src'. 'rootDir' is expected to contain all source files.
3 import ArrowDropDownRounded from '@mui/icons-material/ArrowDropDownRounded';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/branding/brandingTheme.ts:3:34 - error TS6307: File '/Users/janpotoms/Projects/material-ui/packages/mui-icons-material/lib/ArrowDropDownRounded.js' is not listed within the file list of project '/Users/janpotoms/Projects/material-ui/packages/mui-docs/tsconfig.build.json'. Projects must list all files or use an 'include' pattern.
3 import ArrowDropDownRounded from '@mui/icons-material/ArrowDropDownRounded';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Best I could come up with is exclude the icons-material alias (by overriding the paths) and add a declaration file for icons. It feels dirty to me, not sure how you feel about that. Perhaps you think of a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the cleanest solution would be to use the build output of icons-material, but that would yield worse DX.
One other way it could work is to create a single .d.ts file (say, Icon.d.ts) in icons-material with
export { default } from '@mui/material/SvgIcon';
Then to change the paths in tsconfig.json to "@mui/icons-material/*": ["./packages/mui-icons-material/src/Icon.d.ts"],
, so there's a single declaration file covering all the icons. In the future, when we have the exports field in package json, we could even have "./*": { "types": "./src/Icon.d.ts", ... }
and get rid of generating a separate .d.ts per icon (but it's just a random thought, I haven't tested it).
packages/mui-docs/package.json
Outdated
"@mui/material": "workspace:*", | ||
"@mui/base": "workspace:*", | ||
"@mui/icons-material": "workspace:*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we loosen the *
requirement? If @mui/docs is released less often than these packages, pnpm will complain as the versions of installed peer dependencies won't match anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSB CI seems not to convert the workspace protocol to actual version numbers (see package.json in https://pkg.csb.dev/mui/material-ui/commit/37466089/@mui/docs).
I don't know about pnpm's publish
, but CSB itself is a good enough reason to avoid it. Let's use npm-readable version ranges instead (or possibly even "*"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 CBS should do it, it does it with the other packages.
edit: oh right, not for peer dependencies
I'd appreciate if you could verify if installing this via npm works well (AFAIK only the Base UI repo uses it this way now) |
nvm, I opened a canary PR on the base UI repo |
Co-authored-by: Michał Dudak <michal.dudak@gmail.com> Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
mui#41206) Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com> Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
mui#41206) Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com> Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
mui#41206) Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com> Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Moving
brandingTheme
andBrandingProvider
andInfoCard
to the@mui/docs
package