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

breaking: Bump @material-ui/core peer version to v4 #119

Closed
wants to merge 4 commits into from
Closed

breaking: Bump @material-ui/core peer version to v4 #119

wants to merge 4 commits into from

Conversation

eps1lon
Copy link

@eps1lon eps1lon commented May 14, 2019

Breaking change

  • Switch from Slide to Grow as the default transition component
  • Use direct ref on Snackbar instead of StrictMode deprecated RootRef

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.

@jshearer
Copy link

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 build directory isn't included in version control. Is this blocked on anything? Anything I can help with?

@iamhosseindhv
Copy link
Owner

iamhosseindhv commented May 19, 2019

@jshearer this will be merged in a week or so. Meanwhile, you can run npm run prepublishOnly to generate the build directory. And then maybe npm link notistack.

@jshearer
Copy link

Okay great, that's what I ended up going. Thanks @iamhosseindhv :)

@eps1lon
Copy link
Author

eps1lon commented May 20, 2019

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.

You don't really need this PR. Not sure if there's an API for it but if you override the used TransitionComponent to use Grow you're already ready for v4.

The rest is just internals that are not needed anymore.

@iamhosseindhv
Copy link
Owner

True, all material props can be overridden.

Copy link

@favna favna left a 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"
Copy link

@favna favna May 26, 2019

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!

Copy link
Author

@eps1lon eps1lon May 27, 2019

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.

Copy link

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
Copy link
Owner

Thanks @eps1lon and @favna.

I think since we're not using almost any v4 exclusive features, it is not worth it to limit devs by requiring a v4.0.1 peer dependency. This PR will be applied for sure, when more users migrate to v4. and use of RootRef is a trade off that we have to make.

Thanks again.

@eps1lon eps1lon deleted the feat/core-v4 branch June 1, 2019 08:05
@oliviertassinari
Copy link
Contributor

oliviertassinari commented Jun 1, 2019

@iamhosseindhv People depending on Material-UI v3 can use an older version of notistack that is compatible.

@eps1lon
Copy link
Author

eps1lon commented Jun 3, 2019

@iamhosseindhv Would you be open to support both versions as peer dependencies i.e. "@material-ui/core": "^3.2.0 || ^4.0.0". We could add a warning that notistack doesn't implement the latest material guidelines by default and is not strict mode compatible. But at least it wouldn't trigger peer dependency warnings when installing.

@iamhosseindhv
Copy link
Owner

Sounds like a good idea @eps1lon . I'll change peer dependency requirements as per your suggestion and also change default transition component to Grow.

@favna
Copy link

favna commented Jun 6, 2019

Like I said before, you could also just specify it as @material-ui/core": ">=3.2.0 <5". IMO it's far cleaner than @material-ui/core": "^3.2.0 || ^4.0.0"

@eps1lon
Copy link
Author

eps1lon commented Jun 6, 2019

Like I said before, you could also just specify it as @material-ui/core": ">=3.2.0 <5". IMO it's far cleaner than @material-ui/core": "^3.2.0 || ^4.0.0"

Is there a difference?

@favna
Copy link

favna commented Jun 6, 2019

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.

@eps1lon
Copy link
Author

eps1lon commented Jun 6, 2019

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 >= misused in peerDependency when authors just assume their package will work with any future version.

@eps1lon
Copy link
Author

eps1lon commented Jun 7, 2019

Sounds like a good idea @eps1lon . I'll change peer dependency requirements as per your suggestion and also change default transition component to Grow.

@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 classnames to clsx which reduces bundle size.

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

Successfully merging this pull request may close these issues.

5 participants