[RFR] Relax material ui version dependency #3102

fzaninotto · 2019-04-05T14:34:48Z

This allows usage of material ui v3, which is backwards compatible with v1 (Firefox 45 aside).

I tested the simple and demo example: they work perfectly.

Now I regret we didn't do this earlier (!)

fzaninotto · 2019-04-05T14:49:37Z

Some tests fail because we tested the implementation rather than the feature. Time to switch to react-testing-library!

oliviertassinari · 2019-04-05T14:53:55Z

Time to switch to react-testing-library

👍 We are migrating from the shallow to the mount API of enzyme in Material-UI.

maicWorkGithub · 2019-04-08T02:27:10Z

Material-UI V4 is coming......

djhi · 2019-04-08T07:29:55Z

Material-UI V4

It's only an alpha for now. We have time :)

oliviertassinari · 2019-04-08T08:19:56Z

v4.0.0-beta.0 should be out this weekend, v4.0.0 should be out before the end of the month.

dzienisz · 2019-04-17T10:42:47Z

@fzaninotto hey, when you will merge this?

This allows usage of material ui v3, which is backwards compatible with v1 (Firefox 45 aside).

ricardovf · 2019-04-17T17:27:58Z

Tried to install it using yarn add marmelab/react-admin#3102/head with no success, can you help?


yarn add v1.13.0
[1/4] 🔍  Resolving packages...
error Can't add "react-admin-lerna": invalid package version undefined.
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.

Edited: Ok, it seems that this is not possible.

When will you merge this?

djhi · 2019-04-17T17:34:07Z

@ricardovf You can't install a monorepo this way

fzaninotto · 2019-04-17T17:36:50Z

I know that many people are waiting for this one. We’re actually working on it, but it has no release date.

andrewkslv · 2019-04-17T19:27:27Z

@fzaninotto did you test capability on production build? I have run into the issue when classNames have wrong order after refreshing the page.

I created a demo repo for it here https://github.com/andrewkslv/react-admin-with-material-ui-next
Production website http://dead-ticket.surge.sh

fzaninotto · 2019-04-18T07:34:31Z

@andrewkslv which issue? I don't understand your point.

And no, we haven't tested that on production yet, as you can see the CI build is still red, we're working on it.

andrewkslv · 2019-04-18T07:46:18Z

@fzaninotto thank you!

I recorded a video with the issue. https://i.imgur.com/xgJQZZj.mp4

...and made it accessible

fzaninotto · 2019-04-18T08:19:53Z

The remaining warnings are deprecation warnings for material-ui. I hope they don't occur in production builds.

fzaninotto · 2019-04-18T08:27:31Z

@andrewkslv I don't know what you're supposed to have and what you're having instead. Please use the issue template to describe your problem in a new issue.

andrewkslv · 2019-04-18T08:48:24Z

@fzaninotto thanks.

djhi · 2019-04-18T10:15:05Z

packages/ra-ui-materialui/src/input/CheckboxGroupInput.spec.js

-                'WithStyles(FormHelperText)'
-            );
-            assert.equal(FormHelperTextElement.length, 0);
+            expect(container.querySelector('p')).toBeNull();


You're testing implentation details here. If the root tag changes for some reason the test will fails but the user won't actually see anything. Those tests may become a pain to maintain. I believe testing the Required field. is not visible is enough

You might use FormHelperTextProps to pass a testid prop if you really really want this

This PR is already a great step in the right direction. I know I'm still testing implementation details every once in a while, but that's also because material-ui makes it somewhat hard.

If the tests about helper text break in the future, I'll fix them. At least, tests about values and label won't break anymore.

Many people are waiting for this change, so I suggest we merge it as is.

fzaninotto · 2019-04-18T17:09:45Z

I'm sorry but I'll have to revert that change. There are 2 reasons:

When using react-admin 2.x with material-ui 3.x, material-ui throws deprecation errors in the console that can't be silenced. There errors are very noisy and hide real developer errors. Besides, depending on your CI settings, they may make the builds fail. The mui team won't consider turning these errors into warnings, and we have no way to silence them in react-admin without breaking compatibility with mui 1.x.
When setting the dependency of react-admin to material-ui ">1.5.0 <4.0.0", even though an app depending on react-admin has a dependency on an old but compatible version (like 1.5.1), yarn installs 2 versions of material-ui (one recent, like 3.5, and one satisfying the old version requirement, like 1.5.1). I have no idea why this happens, but the duplicated libraries end up in the js bundle, almost doubling its size. This doesn't happen if an app has a dependency on a recent version (like 3.5): in that case, yarn correctly deduplicates material-ui. That's what I first did in this PR. But when I tried to downgrade the examples to get rid of the mui 3.x errors, I saw that yarn duplicated the material-ui package. A doubling of the js bundle size is a major bug that we can't publish.

In other terms, allowing react-admin to support both material-ui 1.X and 3.X is NOT possible. I'm sorry if I gave false hope, it's just that I really wanted it to work, too.

We'll just have to wait for material-ui v4, which will require a major rewrite of both react-admin and your apps based on react-admin v2 😞

Refs #3102

eps1lon · 2019-04-18T17:52:04Z

The mui team won't consider turning these errors into warnings,

This is not true. Please don't spread misinformation before everybody had a chance to speak up. We don't just deploy code without any rationale. For anybody interested in why the warnings are logged the way they are I suggest reading the linked issue.

fzaninotto · 2019-04-18T18:31:49Z

This is not true.

@eps1lon You've lost me: I opened issue mui/material-ui#15397 to ask you specifically to turn these errors into logs or warnings, and you closed it. If you actually want to turn deprecation warnings into console warnings, then please reopen the said issue.

I'm curious as to what you consider misinformation, as I linked the issue in my comment. Everyone can make up their mind.

dzienisz · 2019-04-18T21:40:15Z

@fzaninotto can we make react-admin version that will work only on material ui v3?

fzaninotto · 2019-04-19T06:47:29Z

@dzienisz you sure can, by forking the repo. The core team won't do it in the current repository, for reasons explained in the FAQ.

oliviertassinari · 2019-04-25T19:06:08Z

The mui team won't consider turning these errors into warnings, and we have no way to silence them in react-admin without breaking compatibility with mui 1.x.

@fzaninotto To be precise. We won't change the warning strategy in v3. We will improve the strategy in v4: Improve the warning strategy (#15343).

We want the warnings to log the component stack trace. It's a requirement we don't want to give up on. It really helps to identify where the wrong logic is. Handling breaking changes are painful enough, we are using all our leverages to simplify the migration. In order to do that, we have to use the prop-types module. This module happens to use console.error instead of console.warn. Turning the warnings in a console.warn / console.log would mean giving up on the components stack trace while not significantly improving the situation. I don't think that it's OK to logs deprecation messages, flooding the console when following the documentation instructions (the default).

In v4, we want to leverage the new React.warn() method. It logs the component stack trace with console.warn. We would be happy to have your input on the topic, help us shape the future of the library.

allowing react-admin to support both material-ui 1.X and 3.X is NOT possible.

It wasn't designed for. I wouldn't try to do it. What prevents you from dropping Material-UI v1 support? As far as I know, you don't expose Material-UI directly, all the components are wrapped. Could you increase the Material-UI dependency from v1 to v3, fix all the warnings and release the change as a minor?

We'll just have to wait for material-ui v4, which will require a major rewrite of both react-admin and your apps based on react-admin v2 😞

The v1 or v3 (it's the same) migration is detailed in https://next.material-ui.com/guides/migration-v3/. What point is specifically a "major rewrite"?

Refs marmelab#3102

fzaninotto added this to the 2.8.6 milestone Apr 5, 2019

This was referenced Apr 5, 2019

How to use react-admin with the latest version of material-ui? #3075

Closed

Upgrade Material UI to Latest Stable #2399

Closed

djhi approved these changes Apr 5, 2019

View reviewed changes

fzaninotto added 5 commits April 17, 2019 19:19

Relax material ui version dependency

ef5cde1

This allows usage of material ui v3, which is backwards compatible with v1 (Firefox 45 aside).

Remove outdated FAQ

6a24eec

Migrate CheckboxGroupInput unit tests to react-testing-library

972bb84

Migrate BoolanField unit tests to react-testing-library

4fb9f4a

Migrate BooleanInput unit tests to react-testing-library

18a7510

Avoid testing implementation details (svgs may change)

6414b14

djhi force-pushed the allow-material-ui-3 branch from 2f9b484 to 6414b14 Compare April 17, 2019 17:29

Fix after rebase

f467e5b

fzaninotto added 4 commits April 18, 2019 07:41

Migrate SelectarrayInput unit tests to react-testing-library

252fb8d

Add assertion for icon to BooleanField unit test

eb72c7a

Migrate RadioButtonGroupInput unit tests to react-testing-library

ec4e433

Migrate Pagination unit tests to react-testing-library

35bbe47

djhi approved these changes Apr 18, 2019

View reviewed changes

Migrate DateInput unit tests to react-test-ing-library

89dd4a0

...and made it accessible

fzaninotto mentioned this pull request Apr 18, 2019

Turn deprecation warnings into logs mui/material-ui#15397

Closed

2 tasks

djhi reviewed Apr 18, 2019

View reviewed changes

djhi merged commit 4e37a25 into master Apr 18, 2019

djhi deleted the allow-material-ui-3 branch April 18, 2019 15:31

fzaninotto added a commit that referenced this pull request Apr 18, 2019

Revert Relax material ui version dependency

1028418

Refs #3102

fzaninotto mentioned this pull request Apr 18, 2019

[RFR] Revert Relax material ui version dependency #3141

Merged

andrewkslv mentioned this pull request Apr 25, 2019

production build - classnames conflicts mui/material-ui#8223

Closed

1 task

ghodsizadeh pushed a commit to ghodsizadeh/react-admin that referenced this pull request May 21, 2019

Revert Relax material ui version dependency

ce8878b

Refs marmelab#3102

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFR] Relax material ui version dependency #3102

[RFR] Relax material ui version dependency #3102

fzaninotto commented Apr 5, 2019

fzaninotto commented Apr 5, 2019

oliviertassinari commented Apr 5, 2019

maicWorkGithub commented Apr 8, 2019

djhi commented Apr 8, 2019 •

edited

Loading

oliviertassinari commented Apr 8, 2019

dzienisz commented Apr 17, 2019

ricardovf commented Apr 17, 2019 •

edited

Loading

djhi commented Apr 17, 2019

fzaninotto commented Apr 17, 2019

andrewkslv commented Apr 17, 2019

fzaninotto commented Apr 18, 2019

andrewkslv commented Apr 18, 2019

fzaninotto commented Apr 18, 2019

fzaninotto commented Apr 18, 2019

andrewkslv commented Apr 18, 2019

djhi Apr 18, 2019

djhi Apr 18, 2019

fzaninotto Apr 18, 2019

fzaninotto commented Apr 18, 2019

eps1lon commented Apr 18, 2019

fzaninotto commented Apr 18, 2019

dzienisz commented Apr 18, 2019

fzaninotto commented Apr 19, 2019

oliviertassinari commented Apr 25, 2019

[RFR] Relax material ui version dependency #3102

[RFR] Relax material ui version dependency #3102

Conversation

fzaninotto commented Apr 5, 2019

fzaninotto commented Apr 5, 2019

oliviertassinari commented Apr 5, 2019

maicWorkGithub commented Apr 8, 2019

djhi commented Apr 8, 2019 • edited Loading

oliviertassinari commented Apr 8, 2019

dzienisz commented Apr 17, 2019

ricardovf commented Apr 17, 2019 • edited Loading

djhi commented Apr 17, 2019

fzaninotto commented Apr 17, 2019

andrewkslv commented Apr 17, 2019

fzaninotto commented Apr 18, 2019

andrewkslv commented Apr 18, 2019

fzaninotto commented Apr 18, 2019

fzaninotto commented Apr 18, 2019

andrewkslv commented Apr 18, 2019

djhi Apr 18, 2019

Choose a reason for hiding this comment

djhi Apr 18, 2019

Choose a reason for hiding this comment

fzaninotto Apr 18, 2019

Choose a reason for hiding this comment

fzaninotto commented Apr 18, 2019

eps1lon commented Apr 18, 2019

fzaninotto commented Apr 18, 2019

dzienisz commented Apr 18, 2019

fzaninotto commented Apr 19, 2019

oliviertassinari commented Apr 25, 2019

djhi commented Apr 8, 2019 •

edited

Loading

ricardovf commented Apr 17, 2019 •

edited

Loading