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

[base] Remove unstyled suffix from Base components + Codemod script #36873

Merged
merged 29 commits into from
Apr 24, 2023

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Apr 13, 2023

@hbjORbj hbjORbj self-assigned this Apr 13, 2023
@hbjORbj hbjORbj added package: codemod Specific to @mui/codemod package: base-ui Specific to @mui/base labels Apr 13, 2023
@mui-bot
Copy link

mui-bot commented Apr 13, 2023

Netlify deploy preview

@material-ui/core: parsed: +0.10% , gzip: +0.06%
@material-ui/lab: parsed: +0.16% , gzip: +0.08%
@material-ui/unstyled: parsed: -0.37% 😍, gzip: -0.04% 😍

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 1d1b84a

@hbjORbj hbjORbj force-pushed the base/codemod-unstyled-suffix branch from 0f9e9fc to 93ef82e Compare April 17, 2023 12:26
@hbjORbj hbjORbj changed the title [codemod][base] Codemod script for removal of unstyled suffix [base] Remove unstyled suffix from Base components + Codemod script Apr 17, 2023
@hbjORbj hbjORbj marked this pull request as draft April 17, 2023 13:34
@hbjORbj hbjORbj force-pushed the base/codemod-unstyled-suffix branch from 93ef82e to 4ef2b4c Compare April 17, 2023 13:37
@hbjORbj hbjORbj requested a review from mnajdova April 18, 2023 10:10
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Without renaming the folders in the packages/mui-base/src everything is broken, as the imports are not updated from '@mui/base/ButtonUnstyledto@mui/base/Button, while the folder would still be named ButtonUnstyled`.

@hbjORbj hbjORbj marked this pull request as ready for review April 18, 2023 13:20
@hbjORbj hbjORbj force-pushed the base/codemod-unstyled-suffix branch 5 times, most recently from 3dcf0d0 to 9ed3c1b Compare April 18, 2023 23:41
@hbjORbj hbjORbj requested a review from mnajdova April 18, 2023 23:48
@hbjORbj hbjORbj force-pushed the base/codemod-unstyled-suffix branch 6 times, most recently from cc46fac to f82f860 Compare April 19, 2023 11:57
@mnajdova
Copy link
Member

The failing test is related to #36936 (it's one of the problems that is solved).

@mnajdova
Copy link
Member

mnajdova commented Apr 21, 2023

On the codemod, I would expect that interface and other exported modules, like classes etc. that are renamed would be aliased as the old ones, or we should replace them in the whole file, for e.g.

 import ButtonUnstyled, {
-  ButtonUnstyledProps,
+  ButtonProps as ButtonUnstyledProps,
-  ButtonUnstyledRootSlotProps,
+  ButtonRootSlotProps as as ButtonUnstyledRootSlotProps,,
-  buttonUnstyledClasses,
+  buttonClasses as buttonUnstyledClasses,
-} from '@mui/base/ButtonUnstyled';
+} from '@mui/base/Button';

We should also add test if some of these is already aliased before the codemod.

@mnajdova
Copy link
Member

mnajdova commented Apr 21, 2023

The logic in

function getUnstyledFilename(filename, definitionFile = false) {
is broken. We should check where it is used and find alternative implementation

Update: This was used when the Material UI components were inheriting from the unstyled components, however they are now moved to use the hooks, we can drop this logic altogether. Probably would be better if we isolate this change in a new PR.

@hbjORbj hbjORbj force-pushed the base/codemod-unstyled-suffix branch from d5ad501 to 097e9e5 Compare April 24, 2023 08:20
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 24, 2023
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The codmod updates look great 👌

packages/api-docs-builder/buildApiUtils.ts Show resolved Hide resolved
packages/api-docs-builder/buildApiUtils.ts Show resolved Hide resolved
packages/api-docs-builder/utils/replaceUrl.test.js Outdated Show resolved Hide resolved
packages/api-docs-builder/utils/replaceUrl.ts Outdated Show resolved Hide resolved
@mnajdova
Copy link
Member

mnajdova commented Apr 24, 2023

@siriwatknp or @Janpot maybe you can help. We are removing the Unstyled suffix from the Base UI's components in this PR. Previously the API links looked like: https://mui.com/base/react-button/components-api/#button-unstyled, but now as the unstyled suffix is gone, the anchor is different: https://mui.com/base/react-button/components-api/#button. I pushed 35325a7 in order to fix this, however if you open https://deploy-preview-36873--material-ui.netlify.app/base/react-button/components-api/#button-unstyled you will notice that the replacement of the url is happening twice. Do you know what may be the reason for this and how can we avoid it?

url-unstyled

It is not that big a deal, as these old URLs were there for only two weeks, we can likely drop this logic after some time, but it bugs me a lot.

cc @oliviertassinari it was only two weeks and we had to become creative to fix a redirects with the anchors :)

@Janpot
Copy link
Member

Janpot commented Apr 24, 2023

@mnajdova Maybe try

if (router.isReady && anchor && anchor.endsWith('-unstyled')) {

From https://nextjs.org/docs/advanced-features/automatic-static-optimization#caveats

Avoid using the asPath value on next/router in the rendering tree until the router's isReady field is true.


To add a small nit that I noticed I'd also do

hash: `${anchor.slice(0, -'-unstyled'.length)}`,

To avoid replacing the string "-unstyled" should it appear in the middle of the url for some reason. (Probably won't happen in practice?)

@mnajdova
Copy link
Member

mnajdova commented Apr 24, 2023

@mnajdova Maybe try

if (router.isReady && anchor && anchor.endsWith('-unstyled')) {

From https://nextjs.org/docs/advanced-features/automatic-static-optimization#caveats

Avoid using the asPath value on next/router in the rendering tree until the router's isReady field is true.

Aaaah yes, this was it, thanks Jan!

To add a small nit that I noticed I'd also do

hash: `${anchor.slice(0, -'-unstyled'.length)}`,

To avoid replacing the string "-unstyled" should it appear in the middle of the url for some reason. (Probably won't happen in practice?)

Applied the change just to safe, however it shouldn't happen.

Actually I needed to keep the replacement of the -unstyled suffix anywhere in the anchor and updated the condition to just check if the -unstyled string is present, because we have anchors like #button-unstyled-props => #button-props

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I can't spot anything else, we should be ready for merge 👍

@hbjORbj hbjORbj merged commit e555828 into mui:master Apr 24, 2023
5 checks passed
@michaldudak michaldudak added this to the MUI Base stable release milestone Apr 25, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 28, 2023

I have found a regression.

  1. Open https://deploy-preview-36873--material-ui.netlify.app/material-ui/react-slider/#api
  2. The API link points to Base UI but it should point to Material UI

Screenshot 2023-04-28 at 17 50 18

Can we solve this as a high priority? There are 20 links like this that point developers to the wrong place. Source. Thanks

To be fair, I doubt that this PR is the root of the problem, but just a change that makes is more obvious. While this link wasn't broken before, the first that I can find broken starts from #35938.

@tisonkun
Copy link

tisonkun commented May 9, 2023

This is a breaking change, while it may be acceptable as @mui/base is in alpha, but other @mui components depend on it.

See apache/pulsar-site#566.

tisonkun added a commit to apache/pulsar-site that referenced this pull request May 9, 2023
This is caused by mui/material-ui#36873.

Signed-off-by: tison <wander4096@gmail.com>
@oliviertassinari
Copy link
Member

oliviertassinari commented May 9, 2023

@tisonkun

  1. Do you have an example where this change has an impact beyond @mui/base? It wasn't intended to be a breaking change to any stable npm package.
  2. What your thoughts on the change. Does the new names improve your DX?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change package: base-ui Specific to @mui/base package: codemod Specific to @mui/codemod
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

None yet

7 participants