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.

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] Bump monorepo #14141

Merged
merged 15 commits into from
Aug 14, 2024
Merged

[core] Bump monorepo #14141

merged 15 commits into from
Aug 14, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Aug 8, 2024

Closes #14084.

Just realized I made a few breaking changes in the eslint setup.

@LukasTy Just have two errors on nested imports from the pickers package. How would you prefer to deal with those? just Ignore? Disable in config? Or move the export up one level?

@Janpot Janpot added the dependencies Update of dependencies label Aug 8, 2024
@mui-bot
Copy link

mui-bot commented Aug 8, 2024

@alexfauquette
Copy link
Member

alexfauquette commented Aug 12, 2024

I updated the PR to

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

I've removed the introduction of type exporting by removing the internal type usage in documentation page component.

@LukasTy
Copy link
Member

LukasTy commented Aug 12, 2024

@Janpot Is there anything else we are waiting for this PR to be ready?

@Janpot
Copy link
Member Author

Janpot commented Aug 12, 2024

Is there anything else we are waiting for this PR to be ready?

I haven't addressed this yet. But I can also look into it separately, doesn't necessarily have to be a blocker.

@LukasTy
Copy link
Member

LukasTy commented Aug 12, 2024

I haven't addressed this yet. But I can also look into it separately, doesn't necessarily have to be a blocker.

It could be due to a misconfigured eslint config on the mui-x side. 🙈
I would vote for ignoring this question, at least for the time being. 👍

@Janpot
Copy link
Member Author

Janpot commented Aug 12, 2024

It could be due to a misconfigured eslint config on the mui-x side.

It's likely that the X version of this rule is overwriting a default somewhere. It's quite complicated to extend this rule. Core wasn't even doing it correctly within its repo. It's not a huge problem, I saw that X has made a very detailed setup, I'd like to bring some of this infra to core.

@Janpot
Copy link
Member Author

Janpot commented Aug 13, 2024

@LukasTy
Ok, I touched up the no-restricted-import rule and this brought up a few issues. Fixed most of them. I added exceptions for:

  • @mui/x-date-pickers/internals/demo
  • @mui/x-tree-view/hooks/useTreeViewApiRef
  • @mui/material/ButtonBase/TouchRipple: This will definitely need to be fixed, it'll break once we move to package exports

In general I'd recommend against deeper imports, we're having this convention, It'll be easier to manage our linting rules and build tools if we adhere to it and not have to declare exceptions every time. I'd keep it for when there's a real good reason to do so.

Found an issue where @mui/monorepo/docs/... was imported instead of the alias docs/... and found that the alias was incorrectly defined. that's why // @ts-ignore was used everywhere this was imported. This is fixed now as well

@LukasTy
Copy link
Member

LukasTy commented Aug 13, 2024

Nice, thanks @Janpot, looks great! 👏 💙

@alexfauquette
Copy link
Member

The TS error import ... from 'packages/api-docs-builder'; does not exist

Has been fixed in mui/material-ui#43199 just merged so bumping could solve some errors.

But most of them are from missing type for react-docgen. Those types are defined here: https://github.com/mui/material-ui/tree/next/packages/react-docgen-types

But I'm not sure if it should be fixed on the core repository, or if tsconfig should point to the monorepo on X side

@LukasTy
Copy link
Member

LukasTy commented Aug 13, 2024

But I'm not sure if it should be fixed on the core repository, or if tsconfig should point to the monorepo on X side

Without a too-deep investigation, I think we should add this path to the types in the tsconfig on the X side.

@alexfauquette
Copy link
Member

I tried "@types/react-docgen": ["./node_modules/@mui/monorepo/packages/react-docgen-types"], in multiple places without success

@Janpot
Copy link
Member Author

Janpot commented Aug 13, 2024

I don't think tsconfig paths apply to anything else than project files, so it's in code that we will need to reference this @types/react-docgen that we supplied in the paths. Here I'm doing it in a docs-env.d.ts that I introduce to the compilation in the tsconfig include.
Also pulled in some fixes from PR mui/material-ui#43285. To be merged first.

@@ -1,7 +1,8 @@
import * as React from 'react';
import { PieChart } from '@mui/x-charts/PieChart';

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't remove this newline for some reason. But didn't take the time to look into it.

Copy link
Member

Choose a reason for hiding this comment

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

This behavior is known and we tend to ignore it. 🙈
P.S. I take it that adding type after import doesn't change anything? 🤔

Copy link
Member Author

@Janpot Janpot Aug 14, 2024

Choose a reason for hiding this comment

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

It signals compilers that this import is not usable as a javascript value. Can be important depending on the context this is compiled in.

@Janpot Janpot marked this pull request as ready for review August 14, 2024 07:29
@Janpot
Copy link
Member Author

Janpot commented Aug 14, 2024

Should be ready to review. Have a small comment that we could still resolve. I'm a bit allergic to as any 😄

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for the effort once again. 🙏

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Thanks for all the fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants