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

Why was a breaking change introduced in a patch version? #17918

Closed
RareSecond opened this issue Oct 17, 2019 · 7 comments
Closed

Why was a breaking change introduced in a patch version? #17918

RareSecond opened this issue Oct 17, 2019 · 7 comments
Labels
status: waiting for author Issue with insufficient information

Comments

@RareSecond
Copy link

Here are some highlights ✨:

πŸ“š Change imports from @material-ui/styles to @material-ui/core/styles (#17447) @mnemanja

The presence of two almost identical import paths has been a source of confusion: @material-ui/styles and @material-ui/core/styles. Starting with v4.5.1, the documentation mentions @material-ui/core/styles as much as possible.

-import { makeStyles } from '@material-ui/styles';
+import { makeStyles } from '@material-ui/core/styles';
This change removes the need to install the @material-ui/styles package directly. It prevents the duplication of @material-ui/styles in bundles and avoids confusion. You can learn more about the difference in the documentation.

For one of our repo's, this turned out to be a breaking change. Why was this introduced in a patch version? Not really following semver is super impactful..

@eps1lon
Copy link
Member

eps1lon commented Oct 17, 2019

Starting with v4.5.1, the documentation mentions @material-ui/core/styles as much as possible.

Emphasis on the documentation. The PR should not affect existing code with a breaking change.

Could you include an example that is working different now?

@eps1lon eps1lon added the status: waiting for author Issue with insufficient information label Oct 17, 2019
@oliviertassinari
Copy link
Member

I suspect a duplicate of #17900.

@RareSecond
Copy link
Author

Thing is, builds were still working, so I don't think it's related to that issue.
It was still running correctly, but injectFirst stopped working.

I'll try to create a minimal repo tomorrow.

@oliviertassinari
Copy link
Member

@JDansercoer Oh, then it's likely a duplicated @material-ui/styles module in your bundle. It's an issue that hit people since v4.0.0. The change you have linked to was precisely designed to prevent it from happening in the first place. I'm closing, until we have a reproduction.

@RareSecond
Copy link
Author

All right, here's my example @oliviertassinari.

First the working example: https://codesandbox.io/embed/bold-yalow-jjg30
Then the broken example: https://codesandbox.io/embed/musing-snowflake-4oxbe

As you can see, the second button should be red, while in the broken example, MUI styles are taking precedence again.

Only change I did, was upgrade material-ui/core from 4.3.1 to 4.5.1. material-ui/styles remained locked at 4.4.1.

Changing the import from import { StylesProvider } from "@material-ui/styles"; to import { StylesProvider } from "@material-ui/core/styles"; fixes the issue in the broken example, but me requiring to change imports is something I consider a breaking change.

@oliviertassinari
Copy link
Member

Only change I did, was upgrade material-ui/core from 4.3.1 to 4.5.1. material-ui/styles remained locked at 4.4.1.

You need to upgrade both at the same time. You would experience the same problem with any of our releases since v4.0.0.

@eps1lon
Copy link
Member

eps1lon commented Oct 18, 2019

Only change I did, was upgrade material-ui/core from 4.3.1 to 4.5.1. material-ui/styles remained locked at 4.4.1.

Could you post a yarn why @material-ui/styles to make sure there's actually only one version.

Codesandbox does not resolve @material-ui/styles to a single version (see console warning).

This is an unfortunate side-effect of how react handles context. If provider and consumer are imported from different modules (here: different files and versions) then the consumer will not read the value from the provider but the default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Issue with insufficient information
Projects
None yet
Development

No branches or pull requests

3 participants