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

[Alert] Move from lab to core #22651

Merged
merged 18 commits into from
Sep 23, 2020
Merged

[Alert] Move from lab to core #22651

merged 18 commits into from
Sep 23, 2020

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Sep 19, 2020

Breaking changes

  • [Alert] Move the component from the lab to the core. The component will become stable.

    -import Alert from '@material-ui/lab/Alert';
    -import AlertTitle from '@material-ui/lab/AlertTitle';
    +import Alert from '@material-ui/core/Alert';
    +import AlertTitle from '@material-ui/core/AlertTitle';

@mbrookes mbrookes added breaking change component: alert This is the name of the generic UI component, not the React module! labels Sep 19, 2020
@mbrookes mbrookes added this to the v5 milestone Sep 19, 2020
@oliviertassinari
Copy link
Member

@mbrookes What do you think about we re-export the Alert in the lab from the core up until v5.1.0. This would avoid forcing developers to rename imports when they update the dependency. Making the migration a bit smoother.

@mbrookes
Copy link
Member Author

mbrookes commented Sep 19, 2020

I was going to do that for 4.x (with deprecation warnings), but sure, we could do that here too.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 19, 2020

@mbrookes What's the advantage of doing it for v4? I think that we could only handle v5, the components are unstable in v4. Would making the lab components stable in v4 conflict with the objective to make them stable as an incentive to upgrade? What about if we need to make a breaking change to them?

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 19, 2020

@material-ui/core: parsed: +1.34% , gzip: +0.98%
@material-ui/lab: parsed: +0.36% , gzip: -0.04% 😍

Details of bundle changes

Generated by 🚫 dangerJS against 2554cad

@mbrookes
Copy link
Member Author

mbrookes commented Sep 19, 2020

What's the advantage of doing it for v4?

Deprecation warnings.

Would making the lab components stable in v4 conflict

Not stable in core v4, just deprecated in lab v4 (when lab v5 is release), but perhaps that's overkill?

@mbrookes

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@mbrookes

This comment has been minimized.

@mbrookes
Copy link
Member Author

mbrookes commented Sep 20, 2020

Why the heck is the API docs generator script checking for the presence of unit tests?

@eps1lon
Copy link
Member

eps1lon commented Sep 21, 2020

Why the heck is the API docs generator script checking for the presence of unit tests?

To verify that the documented behavior is actually implemented.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I would not recommend duplicating the component to prevent bundling it twice. We already introduce breaking changes to apply consistent prop naming so I don't think we need to start being careful now. Especially because imports are codemod-able.

Then we automatically avoid potential documentation conflicts by having one component in two packages.

packages/material-ui-lab/src/Alert/Alert.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 22, 2020
@mbrookes
Copy link
Member Author

What's the plan here?

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 23, 2020
@@ -3,6 +3,9 @@ import Alert from '@material-ui/core/Alert';

let warnedOnce = false;

/**
* @ignore - do not document.
Copy link
Member Author

@mbrookes mbrookes Sep 23, 2020

Choose a reason for hiding this comment

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

Oh, of course! 🤦

@oliviertassinari
Copy link
Member

From my perspective, we don't need any backport for this in v4, nor will we for any of the other components we move from the lab to the core. Making the import from lab still work already fulfill the objective of making the v4 -> v5 switch easy.

@mbrookes
Copy link
Member Author

mbrookes commented Sep 23, 2020

Yes – some of the components have been in the lab for so long, I almost forgot that it's perpetually breaking! 🙂

@mbrookes mbrookes merged commit ba8ed3e into mui:next Sep 23, 2020
@povilass
Copy link
Contributor

povilass commented Oct 13, 2020

Missing https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/styles/components.d.ts MuiAlert from material-ui/labs typescript in v5.0.0-alpha.12

@oliviertassinari
Copy link
Member

@povilass Thanks for raising it. Do you want to submit a pull request to fix it?

@mbrookes mbrookes deleted the alert-move-core branch October 13, 2020 11:13
@povilass
Copy link
Contributor

#23028 here you go @oliviertassinari

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: alert This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants