-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Only define aliases once for TypeScript and once for Webpack + Babel #35197
Changes from 6 commits
a2d3a00
2be4949
71e2187
fbc0d82
3919518
766651b
c0b7b84
3b3fbe3
02f3b48
6ad4199
0bde12f
88bf1d0
ebd74cf
78b17d7
176d98a
ab98a0c
ac3878d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ const pkg = require('../package.json'); | |
const withDocsInfra = require('./nextConfigDocsInfra'); | ||
const { findPages } = require('./src/modules/utils/find'); | ||
const { LANGUAGES, LANGUAGES_SSR, LANGUAGES_IGNORE_PAGES } = require('./src/modules/constants'); | ||
const getMuiAliases = require('../scripts/muiAliases'); | ||
|
||
const workspaceRoot = path.join(__dirname, '../'); | ||
|
||
|
@@ -95,7 +96,7 @@ module.exports = withDocsInfra({ | |
}, | ||
], | ||
}, | ||
// transpile 3rd party packages with dependencies in this repository | ||
// Transpile dependencies outside this repository with dependencies in this repository | ||
{ | ||
test: /\.(js|mjs|jsx)$/, | ||
resourceQuery: { not: [/raw/] }, | ||
|
@@ -111,22 +112,7 @@ module.exports = withDocsInfra({ | |
[ | ||
'babel-plugin-module-resolver', | ||
{ | ||
alias: { | ||
// all packages in this monorepo | ||
'@mui/material': '../packages/mui-material/src', | ||
'@mui/docs': '../packages/mui-docs/src', | ||
'@mui/icons-material': '../packages/mui-icons-material/lib', | ||
'@mui/lab': '../packages/mui-lab/src', | ||
'@mui/styled-engine': '../packages/mui-styled-engine/src', | ||
'@mui/styles': '../packages/mui-styles/src', | ||
'@mui/system': '../packages/mui-system/src', | ||
'@mui/private-theming': '../packages/mui-private-theming/src', | ||
'@mui/utils': '../packages/mui-utils/src', | ||
'@mui/base': '../packages/mui-base/src', | ||
'@mui/material-next': '../packages/mui-material-next/src', | ||
'@mui/joy': '../packages/mui-joy/src', | ||
}, | ||
// transformFunctions: ['require'], | ||
alias: getMuiAliases('src'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it seems. In Toolpad we don't use typescript paths, but do aliasing directly in webpack. I believe then it gets picked up under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the core these aliases are needed to make sure that when we use X components on the marketing werbsite, the But if we have a way to say to Next to also resolve some node modules with the TS aliases, that would be great There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not with TS aliases I believe. But with webpack aliases it seems to work that way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we don't have any webpack alias defined no ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably won't be able to get rid of the typescript paths soon as it will be necessary for vscode. But I'd already consider it a win if we can remove them from The real solution is to update our build process in a way that we can rely on yarn workspace linking to resolve local packages. That way we can get rid of all aliasing. There's other obstacles though that currently block us from going there (mainly the build folders that contain the published packages with their subpath exports). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I struggle to apply the webpack aliases on the node_modules, if you have an idea on how to do it (to be able to drop babel which is necessary for notistack and the X deps) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated #33199 and it still seems to work. Check out the changes to next.config.js without whitespace: https://github.com/mui/material-ui/pull/33199/files?diff=split&w=1#diff-97081d4c5e67d7fb92c87085affbac02322e2b4c53c01c5e65f500fc3b67a402 and babel.config.js: https://github.com/mui/material-ui/pull/33199/files?diff=split&w=1#diff-2ed4f5b03d34a87ef641e9e36af4a98a1c0ddaf74d07ce93665957be69b7b09a Will give a quick try if I can replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK but I guess it will only works with the doc, we will still have to keep babel for the testing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sorry for the confusion, I'm only talking about the docs here. It's the only thing we have to deal with from Toolpad's perspective |
||
}, | ||
], | ||
], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,8 @@ | |
"include": ["./src/**/*.ts*"], | ||
"exclude": ["src/**/*.spec.ts*", "src/**/*.test.ts*"], | ||
"references": [ | ||
{ "path": "../mui-utils/tsconfig.build.json" }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to fix the |
||
{ "path": "../mui-base/tsconfig.build.json" }, | ||
{ "path": "../mui-material/tsconfig.build.json" }, | ||
{ "path": "../mui-system/tsconfig.build.json" } | ||
{ "path": "../mui-material/tsconfig.build.json" } | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import { Requireable } from 'prop-types'; | ||
|
||
declare const integerPropType: Requireable<number>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TS was complaining that |
||
export default integerPropType; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
const path = require('path'); | ||
|
||
/** | ||
* @param {'build' | 'src'} type Define if the target should be the built version or the source. | ||
* @param {boolean} isRelative If `true` the path will be relative to the repository root. | ||
*/ | ||
function getMuiAliases(type, isRelative = false) { | ||
flaviendelangle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const workspaceRoot = path.join(__dirname, '..'); | ||
|
||
const resolveAliasPath = (aliasPath) => { | ||
const fullPath = path.join(workspaceRoot, aliasPath); | ||
|
||
if (!isRelative) { | ||
return fullPath; | ||
} | ||
|
||
const resolvedPath = path.relative(process.cwd(), fullPath); | ||
return `./${resolvedPath.replace('\\', '/')}`; | ||
}; | ||
|
||
return { | ||
'@mui/base': resolveAliasPath(`packages/mui-base/${type}`), | ||
'@mui/docs': resolveAliasPath(`packages/mui-docs/${type}`), | ||
'@mui/icons-material': resolveAliasPath( | ||
`packages/mui-icons-material/${type === 'src' ? 'lib' : 'build'}`, | ||
), | ||
'@mui/joy': resolveAliasPath(`packages/mui-joy/${type}`), | ||
'@mui/lab': resolveAliasPath(`packages/mui-lab/${type}`), | ||
'@mui/material': resolveAliasPath(`packages/mui-material/${type}`), | ||
'@mui/material-next': resolveAliasPath(`packages/mui-material-next/${type}`), | ||
'@mui/private-theming': resolveAliasPath(`packages/mui-private-theming/${type}`), | ||
'@mui/styled-engine': resolveAliasPath(`packages/mui-styled-engine/${type}`), | ||
'@mui/styled-engine-sc': resolveAliasPath(`packages/mui-styled-engine-sc/${type}`), | ||
'@mui/styles': resolveAliasPath(`packages/mui-styles/${type}`), | ||
'@mui/system': resolveAliasPath(`packages/mui-system/${type}`), | ||
'@mui/utils': resolveAliasPath(`packages/mui-utils/${type}`), | ||
docs: resolveAliasPath('docs'), | ||
modules: resolveAliasPath('modules'), | ||
'typescript-to-proptypes': resolveAliasPath('packages/typescript-to-proptypes/src'), | ||
}; | ||
} | ||
|
||
module.exports = getMuiAliases; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,43 +133,6 @@ module.exports = function setKarmaConfig(config) { | |
envName: 'stable', | ||
}, | ||
}, | ||
// transpile 3rd party packages with dependencies in this repository | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests are passing without it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need it again for the tree view 😆 |
||
{ | ||
test: /\.(js|mjs|jsx)$/, | ||
include: | ||
/node_modules(\/|\\)(notistack|@mui(\/|\\)x-data-grid|@mui(\/|\\)x-data-grid-pro|@mui(\/|\\)x-license-pro|@mui(\/|\\)x-data-grid-generator|@mui(\/|\\)x-date-pickers-pro|@mui(\/|\\)x-date-pickers)/, | ||
use: { | ||
loader: 'babel-loader', | ||
options: { | ||
// We have to apply `babel-plugin-module-resolve` to the files in `@mui/x-date-pickers`. | ||
// Otherwise we can't import `@mui/material` from `@mui/x-date-pickers` in `yarn test:karma`. | ||
sourceType: 'unambiguous', | ||
plugins: [ | ||
[ | ||
'babel-plugin-module-resolver', | ||
{ | ||
alias: { | ||
// all packages in this monorepo | ||
'@mui/material': './packages/mui-material/src', | ||
'@mui/docs': './packages/mui-docs/src', | ||
'@mui/icons-material': './packages/mui-icons-material/lib', | ||
'@mui/lab': './packages/mui-lab/src', | ||
'@mui/styled-engine': './packages/mui-styled-engine/src', | ||
'@mui/styles': './packages/mui-styles/src', | ||
'@mui/system': './packages/mui-system/src', | ||
'@mui/private-theming': './packages/mui-private-theming/src', | ||
'@mui/utils': './packages/mui-utils/src', | ||
'@mui/base': './packages/mui-base/src', | ||
'@mui/material-next': './packages/mui-material-next/src', | ||
'@mui/joy': './packages/mui-joy/src', | ||
}, | ||
transformFunctions: ['require'], | ||
}, | ||
], | ||
], | ||
}, | ||
}, | ||
}, | ||
{ | ||
test: /\.(js|mjs|ts|tsx)$/, | ||
use: { | ||
|
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.
It now uses the TypeScript aliases
https://nextjs.org/docs/advanced-features/module-path-aliases