[core] Only define aliases once for TypeScript and once for Webpack + Babel #35197

flaviendelangle · 2022-11-18T14:40:38Z

Goals

Define the path leading to the packages in as little places as possible to avoid missing aliases and make the architecture easier to understand.
Reduce our usage of babel.config.js for the doc to hopefully one day be able to use SWC (other plugins already have issues like swc support oliviertassinari/babel-plugin-transform-react-remove-prop-types#201 or Include "babel-plugin-react-remove-properties" functionality swc-project/swc#6057)

Side note

@mui/docs and modules contains almost nothing, maybe we could reduce the amount of aliases (and therefore make the overall architecture easier to understand) in the future.

Signed-off-by: Matheus Wichman <[email protected]> Signed-off-by: Olivier Tassinari <[email protected]> Co-authored-by: Olivier Tassinari <[email protected]>

mui-bot · 2022-11-18T14:43:27Z

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against ac3878d

flaviendelangle · 2022-11-21T08:13:26Z

test/karma.conf.js

@@ -133,43 +133,6 @@ module.exports = function setKarmaConfig(config) {
              envName: 'stable',
            },
          },
-          // transpile 3rd party packages with dependencies in this repository


The tests are passing without it
I think it is not needed since we don't have pickers tests on the core anymore

We need it again for the tree view 😆

flaviendelangle · 2022-11-21T08:14:20Z

docs/babel.config.js

@@ -55,13 +34,6 @@ module.exports = {
    'babel-plugin-optimize-clsx',
    // for IE11 support
    '@babel/plugin-transform-object-assign',
-    [


It now uses the TypeScript aliases

https://nextjs.org/docs/advanced-features/module-path-aliases

flaviendelangle · 2022-11-21T08:14:50Z

packages/mui-utils/src/integerPropType.d.ts

@@ -0,0 +1,4 @@
+import { Requireable } from 'prop-types';
+
+declare const integerPropType: Requireable<number>;


TS was complaining that integerPropType had no typing file when I moved to composite build

flaviendelangle · 2022-11-21T08:15:39Z

packages/mui-material-next/tsconfig.build.json

@@ -13,8 +13,8 @@
  "include": ["./src/**/*.ts*"],
  "exclude": ["src/**/*.spec.ts*", "src/**/*.test.ts*"],
  "references": [
+    { "path": "../mui-utils/tsconfig.build.json" },


I had to fix the references in all the packages, otherwise TS was not able to resolve correctly the imports.
Not sure why it is impacted by the new structure

Janpot · 2022-11-22T09:58:49Z

docs/next.config.js

-                        '@mui/joy': '../packages/mui-joy/src',
-                      },
-                      // transformFunctions: ['require'],
+                      alias: getMuiAliases('src'),


~~Now that aliases moved to webpack (through typescript paths), is this aliasing still necessary?~~

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 node_modules as well and this babel plugin should become unnecessary.

For the core these aliases are needed to make sure that when we use X components on the marketing werbsite, the @mui/material (& co) imports from @mui/x-date-pickers / @mui/x-data-grid are using the local one and not the one from their node modules (which causes issues).

But if we have a way to say to Next to also resolve some node modules with the TS aliases, that would be great

But if we have a way to say to Next to also resolve some node modules with the TS aliases, that would be great

Not with TS aliases I believe. But with webpack aliases it seems to work that way

But we don't have any webpack alias defined no ?

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 babel everywhere. Even if that means moving it to webpack, at least it would be where it makes sense.

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

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)

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 next-transpile-modules with the upcoming built-in transpilePackages

OK but I guess it will only works with the doc, we will still have to keep babel for the testing.
That's still a win for sure.

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

oliviertassinari

I didn't notice anything that could go wrong. Nice cleanup with the karma config file.

scripts/muiAliases.js

mnajdova

Overall looks like a great cleanup. I would recommend resolving Olivier's comments about the function args before merging.

siriwatknp · 2023-05-16T02:34:47Z

@flaviendelangle Could you recheck again?

Janpot · 2023-09-21T11:09:55Z

@flaviendelangle Do you have an idea which specific build/test/lint flows break when we remove the babel aliasing? Just for me to know what makes sense to concentrate on next in code infra?

flaviendelangle · 2023-09-21T11:44:23Z

@flaviendelangle Do you have an idea which specific build/test/lint flows break when we remove the babel aliasing? Just for me to know what makes sense to concentrate on next in code infra?

I did not test it in a long time but I'd say the doc building scripts should breaking

Janpot

Great step in the unification of aliases! A potential next lowish hanging fruit could be shifting eslint resolver from webpack to typescript paths through eslint-import-resolver-typescript

Janpot · 2023-09-21T14:04:53Z

webpackBaseConfig.js

-      'typescript-to-proptypes': path.resolve(__dirname, './packages/typescript-to-proptypes/src'),
-      docs: path.resolve(__dirname, './docs'),
-    },
+    alias: getMuiAliases({ type: 'src', useESIcons: true }),


One thing that could be worth a try is reading them from tsconfig

If we can drop the babel aliases, I guess we could have the tsconfig as the single source of truth.
Would you be ok to keep it for a follow up?

Yes absolutely, this just popped in my head

oliviertassinari

The direction looks nice 👍

oliviertassinari · 2023-09-22T00:11:18Z

test/karma.conf.js

          {
            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|@mui(\/|\\)x-tree-view)/,
+            include: /node_modules(\/|\\)@mui(\/|\\)x-tree-view/,
            use: {


We miss notistack here no? https://mui.com/material-ui/react-snackbar/#notistack.

Ah, same for data grid, and all MUI X components no?

This logic is meant to help us test fixes for these components, make sure if we break something in Material UI for MUI X, we know it as soon as we work on the PR, not once Material UI is released and MUI X updates it dependencies.

This logic is meant to help us test fixes for these components, make sure if we break something in Material UI for MUI X, we know it as soon as we work on the PR

But we don't have any test using those components right?
If the tests pass without the alias.
I can add the alias back, but it will never be used.

Janpot · 2024-03-08T13:03:49Z

@flaviendelangle Just stumbled upon this PR again and realized it has been superseded by #40792 (or at least part of it). Apologies, I think I forgot about the existence of this PR while building that.

flaviendelangle · 2024-03-08T13:40:09Z

I need to go back to it and see if some of it still make sense 😆

oliviertassinari and others added 6 commits April 18, 2022 11:47

disable indexation

a2d3a00

Merge branch 'master' into next

2be4949

Merge branch 'master' into next

71e2187

[docs] Add redirects for MUI X v6 (mui#34296)

fbc0d82

Signed-off-by: Matheus Wichman <[email protected]> Signed-off-by: Olivier Tassinari <[email protected]> Co-authored-by: Olivier Tassinari <[email protected]>

[docs] Handle static files

3919518

[core] Use TypeScript aliases instead of Babel aliases for the doc

766651b

flaviendelangle self-assigned this Nov 18, 2022

flaviendelangle added the core Infrastructure work going on behind the scenes label Nov 18, 2022

flaviendelangle added 5 commits November 18, 2022 16:12

Enable composite build

c0b7b84

Work

3b3fbe3

Work

02f3b48

Remove more aliases

6ad4199

Work

0bde12f

flaviendelangle changed the title ~~[core] Use TypeScript aliases instead of Babel aliases for the doc~~ [core] Only define aliases one for TypeScript and for Webpack + Babel Nov 18, 2022

flaviendelangle changed the title ~~[core] Only define aliases one for TypeScript and for Webpack + Babel~~ [core] Only define aliases once for TypeScript and once for Webpack + Babel Nov 18, 2022

flaviendelangle commented Nov 21, 2022

View reviewed changes

flaviendelangle marked this pull request as ready for review November 22, 2022 09:06

flaviendelangle requested review from mnajdova, Janpot and oliviertassinari and removed request for Janpot November 22, 2022 09:06

Janpot reviewed Nov 22, 2022

View reviewed changes

oliviertassinari reviewed Nov 22, 2022

View reviewed changes

scripts/muiAliases.js Outdated Show resolved Hide resolved

Merge branch 'next' into alias-babel

88bf1d0

Janpot mentioned this pull request Dec 1, 2022

[docs] Shift aliasing responsibilities from babel to webpack #33199

Closed

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2022

mnajdova approved these changes Dec 13, 2022

View reviewed changes

Merge

ebd74cf

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 21, 2023

Work

78b17d7

flaviendelangle force-pushed the alias-babel branch from 6475ec1 to 78b17d7 Compare September 21, 2023 09:51

flaviendelangle added 2 commits September 21, 2023 11:56

Work

176d98a

Work

ab98a0c

Work

ac3878d

flaviendelangle requested review from Janpot, oliviertassinari and siriwatknp September 21, 2023 12:14

Janpot approved these changes Sep 21, 2023

View reviewed changes

Janpot reviewed Sep 21, 2023

View reviewed changes

oliviertassinari reviewed Sep 22, 2023

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 22, 2023

flaviendelangle requested a review from oliviertassinari November 27, 2023 12:38

flaviendelangle closed this Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Only define aliases once for TypeScript and once for Webpack + Babel #35197

[core] Only define aliases once for TypeScript and once for Webpack + Babel #35197

flaviendelangle commented Nov 18, 2022 •

edited

Loading

mui-bot commented Nov 18, 2022 •

edited

Loading

flaviendelangle Nov 21, 2022

flaviendelangle Sep 21, 2023

flaviendelangle Nov 21, 2022

flaviendelangle Nov 21, 2022

flaviendelangle Nov 21, 2022

Janpot Nov 22, 2022 •

edited

Loading

Janpot Nov 22, 2022 •

edited

Loading

flaviendelangle Nov 22, 2022

Janpot Nov 22, 2022

flaviendelangle Nov 22, 2022

Janpot Nov 22, 2022 •

edited

Loading

flaviendelangle Nov 22, 2022

Janpot Nov 23, 2022

flaviendelangle Nov 23, 2022

Janpot Nov 23, 2022 •

edited

Loading

oliviertassinari left a comment

mnajdova left a comment

siriwatknp commented May 16, 2023

Janpot commented Sep 21, 2023

flaviendelangle commented Sep 21, 2023

Janpot left a comment •

edited

Loading

Janpot Sep 21, 2023

flaviendelangle Sep 21, 2023

Janpot Sep 21, 2023 •

edited

Loading

oliviertassinari left a comment

oliviertassinari Sep 22, 2023 •

edited

Loading

flaviendelangle Sep 25, 2023

Janpot commented Mar 8, 2024 •

edited

Loading

flaviendelangle commented Mar 8, 2024

		@@ -0,0 +1,4 @@
		import { Requireable } from 'prop-types';

		declare const integerPropType: Requireable<number>;

[core] Only define aliases once for TypeScript and once for Webpack + Babel #35197

[core] Only define aliases once for TypeScript and once for Webpack + Babel #35197

Conversation

flaviendelangle commented Nov 18, 2022 • edited Loading

Goals

Side note

mui-bot commented Nov 18, 2022 • edited Loading

Netlify deploy preview

Bundle size report

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

Choose a reason for hiding this comment

Janpot Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Janpot Nov 22, 2022 • 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

Janpot Nov 22, 2022 • 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

Janpot Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

siriwatknp commented May 16, 2023

Janpot commented Sep 21, 2023

flaviendelangle commented Sep 21, 2023

Janpot left a comment • 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 21, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot commented Mar 8, 2024 • edited Loading

flaviendelangle commented Mar 8, 2024

flaviendelangle commented Nov 18, 2022 •

edited

Loading

mui-bot commented Nov 18, 2022 •

edited

Loading

Janpot Nov 22, 2022 •

edited

Loading

Janpot Nov 22, 2022 •

edited

Loading

Janpot Nov 22, 2022 •

edited

Loading

Janpot Nov 23, 2022 •

edited

Loading

Janpot left a comment •

edited

Loading

Janpot Sep 21, 2023 •

edited

Loading

oliviertassinari Sep 22, 2023 •

edited

Loading

Janpot commented Mar 8, 2024 •

edited

Loading