-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
breaking: Bump @material-ui/core peer version to v4 #119
Conversation
Hey @eps1lon (or maybe @iamhosseindhv?), I'm just wondering about the status of this PR. We're developing w/ MUI v4 beta, and want to start using notistack. We tried installing this PR directly, but it looks like the |
@jshearer this will be merged in a week or so. Meanwhile, you can run |
Okay great, that's what I ended up going. Thanks @iamhosseindhv :) |
You don't really need this PR. Not sure if there's an API for it but if you override the used The rest is just internals that are not needed anymore. |
True, all material props can be overridden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommending some changes here...
package.json
Outdated
@@ -22,10 +22,10 @@ | |||
"peerDependencies": { | |||
"react": "^16.8.0", | |||
"react-dom": "^16.8.0", | |||
"@material-ui/core": "^3.2.0" | |||
"@material-ui/core": "^4.0.0-beta.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4.0.0 has been released to stable. This should at least be changed to ^4
however if this lib doesn't include any v4 exclusive features (which I think is the case) then I strongly recommend changing it to >=3 <= 5
which will allow any version higher than 3.0.0 and lower than 5 (whenever that releases, and this being a safety net in case 5 breaks the lib).
// other stuff
"peerDependencies": {
"react": ">=16.8.0 < 17",
"react-dom": ">=16.8.0 < 17",
"@material-ui/core": ">=3 < 5"
}
// more stuff
Also gave React the same treatment for the same reasons.
Finally, as backing this suggestion of "Greater Than Or Equal To" versioning let me quote NodeJS' page on Peer Dependencies:
One piece of advice: peer dependency requirements, unlike those for regular dependencies, should be lenient.
You should not lock your peer dependencies down to specific patch versions.
The best way to determine what your peer dependency requirements should be is to actually follow semver.
Assume that only changes in the host package's major version will break your plugin.
Thus, if you've worked with every 1.x version of the host package, use "~1.0" or "1.x" to express this.
If you depend on features introduced in 1.5.2, use ">= 1.5.2 < 2".
Now go forth, and peer depend!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.x would require use of RootRef
. We could support both versions but then this library would trigger warnings in StrictMode and would ultimately be not viable for concurrent react.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, then I would suggest doing ^4
as version for the peer dependency.
@iamhosseindhv People depending on Material-UI v3 can use an older version of notistack that is compatible. |
@iamhosseindhv Would you be open to support both versions as peer dependencies i.e. |
Sounds like a good idea @eps1lon . I'll change peer dependency requirements as per your suggestion and also change default transition component to |
Like I said before, you could also just specify it as |
Is there a difference? |
Technically, no, but I think >= and such is easier to read at a rapid glance and it doesn't require knowledge on how caret versions work in Node since such signs are global. |
The default in package.json is caret and tilde. I mostly see |
@iamhosseindhv But that would be a breaking change at which point I have to ask why don't use that opportunity and switch to v4 fully? That enables strict/concurrent mode compatibility and allows you to switch from |
Breaking change
Breaking changes in
@material-ui/core
v4. We don't have any more breaking changes planned.Tested locally with our docs and it worked as expected.
Sorry for the big package-lock diff. If I know what npm version you used I can maybe reduce it. Initial npm install has its own commit so package-lock diff can be viewed on a per-commit basis.