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

Fix support for next version of @material-ui/core #387

Conversation

mnajdova
Copy link

@mnajdova mnajdova commented May 20, 2021

Changes done:

  • replaces makeStyles, withStyles, createStyles from @material-ui/core/styles with ones from @material-ui/styles. defaultTheme is required to be passed in order to preserve previous behavior.
  • created createTheme util that maps to the legacy createMuiTheme or the new createTheme - reverted these changes see Fix support for next version of @material-ui/core #387 (comment)

Not sure how to test the changes locally, any help is appreciated.

Related to mui/material-ui#26382

@michael-land
Copy link

michael-land commented May 24, 2021

@mnajdova hi, MUI maintainer. Do you have any ideas why custom jss instance or createGenerateClassName in <StyleProvider /> does not work well with notistack?

#389

src/SnackbarContainer.tsx Outdated Show resolved Hide resolved
@mnajdova
Copy link
Author

mnajdova commented Jun 2, 2021

@mnajdova hi, MUI maintainer. Do you have any ideas why custom jss instance or createGenerateClassName in does not work well with notistack?

Not sure without diving too deep. Would be great to test whether v5 will have problems like this, after notistick is compatible with it.

@mlake
Copy link

mlake commented Jun 2, 2021

Hi @mnajdova - i'm happy to help test your code in my own project...

in general should be something like the following

git clone [email protected]:mnajdova/notistack.git 
cd notistack
git checkout feat/core-styles-dependencies-updates
npm install
npm run build
npm link

Then cd to a different project set up with material ui v5...

npm link notistack

the npm link commands help build symbolic links which you can use to develop your code locally without publishing npm packages

@mnajdova
Copy link
Author

mnajdova commented Jun 2, 2021

Thanks @mlake, will try this tomorrow.

@oliviertassinari
Copy link
Contributor

It's probably worth saying that this approach was already tested and released on the data grid with mui/mui-x#1627

@mnajdova
Copy link
Author

mnajdova commented Jun 4, 2021

I've tried testing locally, but there is a problem with the improts.

Basically the import:

import * as styles from '@material-ui/core/styles';

is being transformed to:

import { createMuiTheme, createTheme as createTheme$1, emphasize } from '@material-ui/core/styles';

which then fails as createTheme is not exported from v4. I would revert this to always use the createMuiTheme (it is deprecated in v5, but I don't think there is anything else I can do right now.

@mnajdova
Copy link
Author

mnajdova commented Jun 4, 2021

I tried testing locally, but I got some Invalid hook call. errors, probably didn't link the project correctly, I am not sure. If someone can test a simple scenario whether it work with both v4 and v5 it would be great.

@mlake
Copy link

mlake commented Jun 4, 2021

also got the invalid hook call when I tested..wasn't sure how to go about fixing it

@oliviertassinari
Copy link
Contributor

@iamhosseindhv Do you have guidance on these changes?

@DmitriySot
Copy link

looks like you guys also have no idea from which package you need to import

@oliviertassinari
Copy link
Contributor

@DmitriySot What do you mean?

@DmitriySot
Copy link

DmitriySot commented Jun 6, 2021

I mean that material-ui updated a few weeks ago with some breaking changes related to migration and removing jss. And for me is not too clear from which package I need to import something that will not be deprecated in the next version.And I see that @mnajdova also can't update notistack for that time

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Jun 6, 2021

I need to import something that will not be deprecated in the next version

@DmitriySot The @material-ui/styles package will be deprecated and removed. Related to mui/material-ui#26571

@yurtaev
Copy link

yurtaev commented Jun 7, 2021

I'm just trying to upgrade @material-ui to v5.0.0-alpha.35 on my project. I use yarn patch as workaround to fix notistack. What about releasing the fix as alpha tag? and pin @material-ui/* deps to v5

@mnajdova
Copy link
Author

mnajdova commented Jun 7, 2021

looks like you guys also have no idea from which package you need to import

@DmitriySot comments like this are not really helpful and productive.

I mean that material-ui updated a few weeks ago with some breaking changes related to migration and removing jss. And for me is not too clear from which package I need to import something that will not be deprecated in the next version. And I see that @mnajdova also can't update notistack for that time

We would like to contribute to make notistack compatible with both Material-UI v4 and v5 at the same time, so that it won't break until it is upgraded to v5. Regarding what should we imported from where, we try to list everything in the migration guide, for these specific changes, there is more info https://github.com/mui-org/material-ui/blob/next/docs/src/pages/guides/migration-v4/migration-v4.md#material-uicorestyles Anyway I don't think it's productive to discuss things like this in this PR, you can open an issue for clarifying things.

Again, whoever can help test out notistack with both v4 and v5 would help with merging the PR.

@ocodista
Copy link

ocodista commented Jun 8, 2021

I'm getting an error with the new release of MUI.

styles$2.withStyles is not a function
at Object. (node_modules\notistack\dist\notistack.cjs.development.js:227:47)
at Module._compile (internal/modules/cjs/loader.js:1063:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
at Module.load (internal/modules/cjs/loader.js:928:32)
at Function.Module._load (internal/modules/cjs/loader.js:769:14)
at Module.require (internal/modules/cjs/loader.js:952:19)
at require (internal/modules/cjs/helpers.js:88:18)
at Object. (node_modules\notistack\dist\index.js:7:20)

@yurtaev
Copy link

yurtaev commented Jun 10, 2021

I prepared codesanboxes:

I see the warning for v5:

Material-UI: the createMuiTheme function was renamed to createTheme.

You should use `import { createTheme } from '@material-ui/core/styles'`

@hauptrolle
Copy link
Contributor

@yurtaev Hey, it looks like the spacing between the notifications is missing? 🤔

@belayaShlyapa
Copy link

belayaShlyapa commented Jun 14, 2021

@iamhosseindhv Do we have some news please ?
I'm still stuck by Attempted import error: 'makeStyles' is not exported from '@material-ui/core/styles'. error using material-ui v5.

@michael-land

This comment has been minimized.

@yurtaev
Copy link

yurtaev commented Jul 2, 2021

Note: related #396 to fix #387 (comment)

@deldrid1
Copy link

deldrid1 commented Jul 8, 2021

EDIT: Just use "notistack": "2.0.1-alpha.4", and all is right with MUI v5.0.0-beta.0.

When yarn installing with a dependency key/value pair of "notistack": "https://github.com/mnajdova/notistack.git#82a1975b255b8be6b43b3d61c03bac36366eae47", in package.json, I get the following error emitted:

warning " > [email protected]" has incorrect peer dependency "@material-ui/core@^4.0.0".

On top of that something appears to still be broken - My next.js app complains with

  • Module not found: Can't resolve 'notistack'
  • > import { SnackbarProvider } from 'notistack';

Any suggestions on how to use the MUI v5 beta with this? The Code Sandbox example above appears to play nice...

@iamhosseindhv
Copy link
Owner

iamhosseindhv commented Jul 13, 2021

Apologies for responding so late. Been dealing with Covid and international travel. Thanks @mnajdova for opening the PR. Couple of things I can think of:

  1. Do we have to list @material-ui/styles in peerDependencies now?
  2. If so, I wouldn't merge this into master as it'd force users that are locked with MUI v4 to download the new dependency. Currently the branch structure is as follows:
branch Published with tag Compatible
master latest MUI v4
next next MUI v5

Could this PR be rebased and merged into next branch?

@MasterNeuromancer
Copy link

thanks @iamhosseindhv . I look forward to trying out the new version. Hope all is well.

@lpfrenette
Copy link

lpfrenette commented Jul 14, 2021 via email

@mwhitney-butter
Copy link

"@material-ui/core": "^5.0.0-beta.0"
"notistack": "^1.0.6-next.1"

Failed to compile.

./node_modules/notistack/dist/notistack.esm.js
Attempted import error: 'makeStyles' is not exported from '@material-ui/core/styles'.

thoughts?

@ivanvladimirov
Copy link

"@material-ui/core": "^5.0.0-beta.0"
"notistack": "^1.0.6-next.1"

Failed to compile.

./node_modules/notistack/dist/notistack.esm.js
Attempted import error: 'makeStyles' is not exported from '@material-ui/core/styles'.

thoughts?

That's what this PR is about to fix

@mwhitney-butter
Copy link

"@material-ui/core": "^5.0.0-beta.0"
"notistack": "^1.0.6-next.1"

Failed to compile.

./node_modules/notistack/dist/notistack.esm.js
Attempted import error: 'makeStyles' is not exported from '@material-ui/core/styles'.

thoughts?

That's what this PR is about to fix

what's the timeline for this PR being merged?

@mnajdova
Copy link
Author

Could this PR be rebased and merged into next branch?

I thought there is one branch (master) to support all versions. If this is not the case, I would say the best is to migrate to emotion in the next branch so that there isn't any overhead added with bundling both JSS and emotion.

@mwhitney-butter
Copy link

mwhitney-butter commented Jul 15, 2021

eventually, need a tagged version of notistack on master that uses material-ui v4 with JSS and a tagged version of notistack on master that uses material-ui v5 with emotion so people can choose which version of notistack they want that's in alignment with the version of material-ui theyre using. until notistack next branch gets merged into master, next branch should only have emotion, then when next branch gets merged into master, the release will be the aforementioned tag on master that uses material-ui v5. for now we definitely need this PR on the next branch of notistack so we can use it with material-ui next

Could this PR be rebased and merged into next branch?

I thought there is one branch (master) to support all versions. If this is not the case, I would say the best is to migrate to emotion in the next branch so that there isn't any overhead added with bundling both JSS and emotion.

@mtr1990
Copy link

mtr1990 commented Jul 16, 2021

When will we have a version that supports MUI beta v5?

I've been waiting for this package to update for 2 months but still no results.

@ivanvladimirov
Copy link

When will we have a version that supports MUI beta v5?

I've been waiting for this package to update for 2 months but still no results.

Same here. Good thing my project is still in development. I hope this PR gets merged soon.

@Lure5134
Copy link

Just upgrade to notistack@next and it will work with material UI 5

@vbornand
Copy link

Just upgrade to notistack@next and it will work with material UI 5

It's not totally correct, it works only with MUI >= 5.00-alpha.34:

@ivanvladimirov
Copy link

Material-UI is currently at its second beta of v5 and styling functions are not part of "core" anymore, therefore notistack cannot work with the new version of MUI

@mnajdova
Copy link
Author

@iamhosseindhv I propose updating to the styled() and sx API in the next branch, so that it doesn't depend on two styling solutions (JSS and emotion).

When this PR was introduced, the goal was to make notistack compatible with both v4 and v5 in the transition period. Now that Material-UI is in beta and this was still not merged, I propose we abandon it and focus on updating the next branch. Let me know if you need some help with/instructions with it.

@mnajdova mnajdova closed this Jul 19, 2021
@stass-1
Copy link

stass-1 commented Jul 22, 2021

@iamhosseindhv currently we don't have any opened tickets or pull-requests to track progress with getting notistack work with MUI v5 beta

@iamhosseindhv
Copy link
Owner

@stass-1 #412 is an open issue to make notistack compatible with MUI 5 beta

@th851dan
Copy link

mui5 is released and has a new package name(@mui/material). Do you want to make notistack compatible? @iamhosseindhv

@mnajdova
Copy link
Author

mui5 is released and has a new package name(@mui/material). Do you want to make notistack compatible? @iamhosseindhv

There is a codemod that should enable fast migration to the new package names - https://github.com/mui-org/material-ui/blob/next/packages/mui-codemod/README.md#mui-replace

@iamhosseindhv
Copy link
Owner

Thanks @mnajdova. @th851dan We have #423 for that issue

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.