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

Material-UI not compatible with React 0.14 #1030

Closed
tleunen opened this issue Jul 3, 2015 · 59 comments
Closed

Material-UI not compatible with React 0.14 #1030

tleunen opened this issue Jul 3, 2015 · 59 comments
Labels
core Infrastructure work going on behind the scenes package: material-ui Specific to @mui/material

Comments

@tleunen
Copy link

tleunen commented Jul 3, 2015

Because of the change on the refs that React 0.14 has. I think Material-UI will have quite a lot of issues with almost every component of the library.

the this.refs.XXX will return the DOM node instead of the component, which means we won't be able to call any component functions on it.

@jkruder
Copy link
Contributor

jkruder commented Jul 4, 2015

@tleunen You're on the ball! React 0.14 beta was released today.

@hai-cea I took a look at some of the issues (I found ~50 across 21 files). This could be a good opportunity for a milestone. I was looking at the dialog/dialog-window/overlay files and there's a lot of coupling via this.refs.xxx.yyy() so this may not be a simple task. If you want to make a React 0.14 Compatibility milestone let me know and I can assist in writing issues (I'm sure @tleunen would like to be involved -- I don't want to volunteer him).

@hai-cea
Copy link
Member

hai-cea commented Jul 4, 2015

Thanks guys @tleunen!

I agree, this will cause some problems. What's the alternative? Save the element into a variable inside render?

@jkruder Are you on gitter - https://gitter.im/callemall/material-ui ? Please send me an IM on there and we'll get the ball rolling on organizing issues/milestones.

@tleunen
Copy link
Author

tleunen commented Jul 4, 2015

Most of the time, components should not have public functions. Everything has to be passed with props or when they are mount.

For example, the Dialog component doesn't need show and hide. If it's in the dom, it's displayed, otherwise it's not.

I'm just starting using Material-UI so I'm not much aware of issues with others components, but I think most of them could be rewritten to not have public functions (Anyway there is no other choices).
The thing is, it will be a big breaking change from the current version.

@oliviertassinari
Copy link
Member

@tleunen I can't agree more on the Dialog show and hide methods.

@hai-cea
Copy link
Member

hai-cea commented Jul 4, 2015

@tleunen @oliviertassinari Yeh, I agree with you both. This was a debate we had when designing that component. The issue that we had was the clickaway feature. If open/close was handled in state, then it could worry about closing itself on clickaway. If open/close were passed in as props, everyone using dialog would have to handle clickaway themselves.

Now a middle solution would be to add an onClickAway prop to dialog and let the user of the component open/close.

@oliviertassinari
Copy link
Member

@hai-cea I would suggest to use the same approche as https://github.com/rackt/react-modal. This is basicaly your middle solution.

@mull
Copy link

mull commented Jul 7, 2015

What other thing does this library use this.refs for when interacting with a DOM component than saying getDOMNode()? This change in React only applies to components such as <div/> and <i/> doesn't it? Your custom components can still be accessed by this.refs.xxx as per usual. Correct me if I'm wrong I haven't tried React 0.14 yet, but this same thing came up on HackerNews.

@tleunen
Copy link
Author

tleunen commented Jul 7, 2015

You mean that if React detect the ref is on a custom component, it will return the component instead of the DOM element inside the component?

@mull
Copy link

mull commented Jul 7, 2015

@tleunen that is my understanding. Needs verification :)

@hai-cea
Copy link
Member

hai-cea commented Jul 7, 2015

@mull That would be very nice if that's the case. :)

@jkruder
Copy link
Contributor

jkruder commented Jul 7, 2015

@hai-cea @mull @tleunen Just ran a quick test and ref on a custom (something that extends React.Component) component (this.refs.customComponent) will return a reference to the React component NOT the underlying DOM node. If you have a ref to a DOM node (div/a/img/etc) then this.refs.domRef will return the node.

@hai-cea That being said, I still think it's a good idea to move away from calling methods on this.refs.XXX.

@mull
Copy link

mull commented Jul 7, 2015

@jkruder thanks, glad I wasn't talking out of my ... :)

@hai-cea
Copy link
Member

hai-cea commented Jul 8, 2015

ok Thanks for the doing the research @jkruder. I think we're save to close this. Although, I think we still need to do a 0.14 milestone?

Also, what do you guys think about #1033 ?

@tleunen
Copy link
Author

tleunen commented Jul 8, 2015

I guess we can close it then. But it should be good to rewrite some of the components to remove the need of calling functions on them. It's not how components should work :/

@jkruder
Copy link
Contributor

jkruder commented Jul 8, 2015

@hai-cea Agreed. I'm working on a draft of the proposed work for 0.14 milestone that I'll send to you for feedback.

In regards to #1033, I don't think we should make the jump just yet. I'm all for creating a separate branch where we can convert MUI so that it's compatible with what is proposed for 0.14 and make the components more functional (minimize/eliminate this.refs.XXX.YYY()).

@elgerlambert
Copy link

If the current use of this.ref.xxx doesn't actually break material-ui when used together with react 0.14.0-beta1 then I don't feel the wish to move away from that pattern should block #1033. By making it easier to install material-ui alongside react 0.14.0-beta1 you'll set yourself up to receive early feedback on actual issues that may arise; it's better to receive that feedback when 0.14 is still in beta.

Perhaps a good alternative (that better manages expectations), is to release an alpha/beta/rc version of material-ui on npm that has 0.14.0 as a peer dependency (and is geared towards making material-ui 0.14 compatible). That way it's easier for people to move forward and find/fix any issues that might exist.

@ashtonsix
Copy link

@jkruder Any update on this?

@jkruder
Copy link
Contributor

jkruder commented Aug 25, 2015

@ashtonwar None yet -- I've been focusing on getting some testing established and resolving some external commitments. I should have some time this week to take a look at it and see what we're working with. Some others have already made some attempts at migrating to 0.14.

@ashtonsix
Copy link

Cheers for looking at this. I've seen material-ui-io from #1033. It seems to work for some components (dropdowns, buttons, snackbar) but fall over & die on others (checkbox, slider, toggle). Not aware of any other attempts to migrate.

@oliviertassinari
Copy link
Member

@tleunen Turn out we can still use this.refs.XXX for custom component. Thank @jkruder .

@jkruder
Copy link
Contributor

jkruder commented Aug 30, 2015

No problem; I still believe that it is best to avoid using this.refs.doSomething() where possible.

@newtack
Copy link

newtack commented Sep 11, 2015

Any updates? React JS 0.14 RC 1 just got released and really would like to use Material-UI with it.

@Gregoor
Copy link

Gregoor commented Sep 12, 2015

same here, is there a way we can support the migration to 14?

@ashtonsix
Copy link

I'm using material-ui-io in production - it seems OK.

@rohitahuja
Copy link

+1. Would also like to know the timeline of this update!

@muloka
Copy link

muloka commented Sep 29, 2015

👍

@shaurya947
Copy link
Contributor

We're getting there, folks! See #1751. At this point a little more work is needed to upgrade to the new react-router api.

I would recommend trying out the react-0.14-support branch and reporting any issues with the [React0.14] prefix in the title. Once we can get that branch fully working I shall close this issue (finally!) :)

@justjacksonn
Copy link

Great to hear! Look forward to a final version. I have been working with
Redux, React, react-router, and so far its a pretty nice way to go. Look
forward to incorporating Material UI in to this.

On Tue, Sep 29, 2015 at 1:31 PM, Shaurya Arora notifications@github.com
wrote:

We're getting there, folks! See #1751
#1751. At this point a
little more work is needed to upgrade to the new react-router api.

I would recommend trying out the react-0.14-support branch and reporting
any issues with the [React0.14] prefix in the title


Reply to this email directly or view it on GitHub
#1030 (comment)
.

@carlitux
Copy link

Cool! I will be putting issues if there is anything...

@amagdas
Copy link

amagdas commented Oct 1, 2015

Any news on a release date for React 0.14 support?

@shaurya947
Copy link
Contributor

@amagdas are you aware of the react-0.14-support branch? It's an ongoing effort. Feel free to test it out and report any issues with the [React0.14] prefix

@amagdas
Copy link

amagdas commented Oct 2, 2015

@shaurya947 Yup, I'm aware of it, but I'm not able to install the branch using npm, trying again.
Having some sort of Readme/wiki on how to test this branch using react 0.14 would be good.

@clkao
Copy link

clkao commented Oct 2, 2015

you can either npm link from a clone or do npm i 'git://github.com/callemall/material-ui#react-0.14-support' in your project.

@justjacksonn
Copy link

To be clear you need to do the npm install in the node_modules directory
not the root of your directory
On Oct 2, 2015 08:01, "Chia-liang Kao" notifications@github.com wrote:

you can either npm link from a clone or do npm i 'git://
github.com/callemall/material-ui#react-0.14-support' in your project.


Reply to this email directly or view it on GitHub
#1030 (comment)
.

@shaurya947
Copy link
Contributor

@amagdas this branch is not up on npm yet as it still has some issues and is a work in progress.

You can either do what @clkao said, or, after cloning the repository on your machine, switch to the react-0.14-support branch using git checkout react-0.14-support.

After that when you run npm i in the root directory, all source files get compiled into the lib folder. You can then use this lib folder in your project.

@amagdas
Copy link

amagdas commented Oct 2, 2015

Yup, done this and it works, will give feedback.

@newtack
Copy link

newtack commented Oct 7, 2015

How about leveraging this FB tool to automatically make the changes? https://github.com/facebook/react/blob/master/packages/react-codemod/README.md

@kinolaev
Copy link

kinolaev commented Oct 7, 2015

Look at "Notable bug fixes" in react 0.14 announce (http://facebook.github.io/react/blog/2015/10/07/react-v0.14.html):
"Click events are handled by React DOM more reliably in mobile browsers, particularly in Mobile Safari."
...

@oliviertassinari
Copy link
Member

@kinolaev For more detail: #1732 (comment)

@ovaris
Copy link

ovaris commented Oct 15, 2015

Is react-0.14-support removed?

@oliviertassinari
Copy link
Member

@ovaris I was merged to master.

@carlitux
Copy link

@oliviertassinari when will be available on npm?

@oliviertassinari
Copy link
Member

when will be available on npm

I have no idea when we will have a bug free version. However you can try master branch with npm.

@oliviertassinari
Copy link
Member

Should be fixed with the latest release v0.13.0
Thanks evreybody for your help.

@carlitux
Copy link

@oliviertassinari thanks!

@muloka
Copy link

muloka commented Oct 24, 2015

merci!

@ishwinder
Copy link

Any Ideas if this issue will still exist in the latest version of material-ui ? I can use most of the components but the ones which use this.refs.xxx. e.g if I try to use DatePicker Component, I get an error "Cannot read property 'show' of undefined" and undefined here is this.refs.dialogWindow.

I am on react 0.14.8 and material-ui 0.14.4 ...

@EasonWang01
Copy link

the same error

@topgun743
Copy link

Seems like material-ui does not work with React 0.14.8 and 0.14.9 and that is pathetic.
Webpack issuing lots of weird complaints on my console. Dont understand what to do.

@ashtonsix
Copy link

@topgun743 That's very hurtful of you to describe the excellent work here (given away for free) as pathetic.

Since this issue was opened React 15 was released, which material-ui is compatible with. I suggest upgrading React inside your project.

@zannager zannager added package: material-ui Specific to @mui/material core Infrastructure work going on behind the scenes labels Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes package: material-ui Specific to @mui/material
Projects
None yet
Development

No branches or pull requests