Material-ui: [Proposal] Stateless Material-UI

[alitaheri]

After seeing a huge number of regressions and bugs introduced only because there are 2 sources of truth for most components (state and prop) I have come to the conclusion that state is bad for business.

The Problem

Take a look at all the bugs. I can argue that more than half of them is caused by the complexity of internal logic of the components. And the biggest reason is keeping the state up-to-date with props.

The Solution

No more support for uncontrolled behavior!

How about forms?

1. A simple wrapper can do the trick:

<Stateful ref='input' defaultValue={/*...*/}>
  <CheckBox/>
</Stateful>

you can then call imperative methods on this.refs.input. or handle the on change. This will require standard callback signatures which is a must even without this.

1. Third party projects like Fromsy can solve this issue
2. HOC on user-side. I don't think it is that hard to implement anyway.

Roamap

• [ ] Remove imperative methods from the library (except for focus, damn focus)
• [x] Do something about theme handling to remove all the redundant codes
• [ ] Standardize the API, i.e. value + onChange(event, value) [debatable] and no more!
• [x] Implement a wrapper that consumes the standard API to easily support form actions
• [ ] Brag about the awesomeness of controlled components
• [ ] Remove support for uncontrolled components

@oliviertassinari @mbrookes @newoga I would like to hear your insight on this. Thanks :+1: :+1:

[mbrookes]

Not that formsy-material-ui is particularly complex as most of the hard work is done by formsy-react (and there is plenty of room for improvement - HOC not mixin, ES6 classes, ES6 modules), but from my perspective, if the material-ui form component API was standardised, it would have made implementation much simpler still.

Across the form components, there are at least three variations of the onChange callback parameter order / content, and a mix of whether components can be uncontrolled, or should be controlled - and this isn't stable across releases of material-ui.

So, I am supportive of the idea, but I'm conscious that it would be a breaking change. If it's going to happen, then sooner is better than later I think.

[alitaheri]

then sooner is better than later I think.

I love the sound of that. The sooner it happens I'll have less code to worry about. But if we ease into it everyone will have time to slowly adept and make smaller changes per release.

I you ask me I'm all in for sooner. But i'm not the only user of this library ( ^_^ )

[oliviertassinari]

But if we ease into it everyone will have time to slowly adept and make smaller changes per release.

Yeah, let's wait a bit :grin:.
In the meantime, I think that we have important stuffs to take care of that shouldn't require breaking changes :tada:

• the should component update
• the inline style
• the theming
• ES6 class definition of component
• versioning the documentation

[oliviertassinari]

I agree with you, the simpler our component are, the best it's. Going in this direction sounds like a good idea :+1:.

[newoga]

I'm very much in agreement with this direction (probably not much of a surprise :smile:). I think it makes a lot of sense to externalize the state and allow the developer to decide where they want state stored (ie. a basic stateful wrapper component or something like redux). I'd very much like to write some tooling to better integrate material-ui with redux in some of my personal projects and this would certainly help. :+1:

[oliviertassinari]

I have just discovered https://github.com/acdlite/recompose. That's preaty interesting!
They have:

• a withContext()
• a pure()

That could make our life easier :heart_eyes:.
Regarding the performances of HOC: https://github.com/acdlite/recompose/blob/master/docs/performance.md

What about migrating with this and radium :grin:?

[newoga]

@oliviertassinari Yeah that's a cool toolset. I ran into it earlier and thought it might be useful too. Also that post on performance is very interesting!

[alitaheri]

I really like how Recompose addresses such issues :+1: :+1:

But... We still have 2 concerns with Recompose:

1. They are all function components, you can't ref 'em, for apps it's ok, but for libs that's scary to users.
2. Imperative methods are not forwarded. sure we wanna remove all the imperative methods. but focus guys!

the should component update
the inline style
the theming
ES6 class definition of component
versioning the documentation

Yeah I think code quality matters too :+1: :+1: I want to address the theme for now, We will clean up a whole lot of components with this. I don't think I can continue with my previous huge PR with that huge es-lint rule though :grin:.

@oliviertassinari So have we decided on Radium? Do you think how they traverse the tree can impact performance that badly? :grin:

P.S. We can still consider inheritance. I'll continue my work so we can see how it would turn out :grin:

[oliviertassinari]

1. They are all function components, you can't ref 'em, for apps it's ok, but for libs that's scary to users.

That's not true, for performance reason, they are using a class under the hood for soom recompose function. That's not yet documented.
Plus, they expose a toClass:

Converts a function component to a class component, e.g. so it can be givena ref.
Returns class components as is.

sure we wanna remove all the imperative methods.

Yes, that would be awesome.

but focus guys!

I guess we can use

this.refs.input.refs.input.focus();

instead of

this.refs.input.focus();

@oliviertassinari So have we decided on Radium? Do you think how they traverse the tree can impact performance that badly? :grin:

Well, if we split up our component and implement should component update correctly, that should be ok. I'm considering Radium since it's maintained by a guy working at facebook and it's popular.

[alitaheri]

Alright, I'll study Recompose a bit more.

And I agree with Radium too. It's got good stuff :grin:

[oliviertassinari]

By the way, radium and react-look are using the HOC like approach. So, even if we use the inheritance, we will have to deal with those issues.

[alitaheri]

@oliviertassinari Yeah I guess -_- This is getting hard :tired_face: I have to give it a lot of thought. @newoga You think about this too :grin:

[newoga]

@alitaheri @oliviertassinari Will do. I starting writing a long response earlier before deciding it was too long and most likely wasn't worth the read :laughing:. I'm certainly continuing to give this thought. Like we said before, I believe if we continue to refactor/clean up code in the more obvious areas, things will become clearer and we'll have a better "playground" to experiment with these different approaches and libraries.

I have some more specific thoughts maybe in which the order we can start doing some of the activities on the roadmap. I'll jot some ideas down later and share for feedback.

[alitaheri]

@newoga Sounds good to me, I agree that we should focus on the quality of the code and get rid of all the _magic_ in the code before we can actually work on something new :+1: :+1:

[mbrookes]

Related: #2957

[Kosta-Github]

With the move to a (more) stateless implementation the react alternative preact might be usable, which would bring in a very low bundle footprint of only 3kb: preact.

[newoga]

With the move to a (more) stateless implementation the react alternative preact might be usable, which would bring in a very low bundle footprint of only 3kb: preact.

@Kosta-Github That's an interesting point. Moving to functional components that return JSX technically opens up the possibility for material-ui to be used with frameworks outside of React that use JSX, like preact. :+1:

[nikgraf]

I really like the idea of separating between stateless & stateful component! :heart:

1. How do you imagine the directory structure? We could split it the directories pure & stateful in the same repository/package.
2. Some components have very complex state management. Especially when keyboard navigation is managed (see Datepicker Code). Making them pure would probably require to introduce new function to get to a sensible way of managing what was internal state before e.g. _addSelectedDays in DatePicker should probably become a public callback. The more I think about it, the more I like it. What are your thoughts?
3. I would love to see the API normalised as @alitaheri suggested. I'm wondering what you think about replacing onChange with something else?
   Explanation: onChange is native to React and is called with SynteticEvent that contains the value inside the target. By having onChange(value) or onChange(event, value) we break with the signature of React's onChange API and newcomers need to learn that. It's not a huge big of a deal, but consistent APIs improve the developer experience long term. Why shouldn't we do onChange(event): because we would need to create SynteticEvents while in most cases people only want the value. I suggest onUpdate({ value: <here the real value> }) or also can be onModify or onValueChange. Having an object with value inside allows us to extend it without breaking the API in the future.

[alitaheri]

@nikgraf Thanks for taking part in the discussion :+1: The more brain power we have the better the decision we make :grin:

1. I was thinking more like material-ui/Snackbar and material-ui/Snackbar/stateful. But that's open to discussion we might have more versions than we think O.o
2. I'm working on a library to keep all controlled props inside a wrapper that will work like this:
   
   createWrapper(['value', 'open', 'etc'])(Component) it will create a class with onValueChange, onOpenChange, onEtcChange, defaultValue, defaultOpen, defaultEtc props and clearValue, setValue, getValue, ... methods. That way all we need to to is write a file with a wrapper in it :grin:
3. I'm thinking even a more standard manner. like: onValueChange as I mentioned above.

That wasn't the original reason. it's for integration with the Wrapper. we still pass the event. it won't break their previous knowledge. the thing about passing an object is that it's very uncommon and a bit hard to document and keep track of, but it's something workth discussing :+1: Thanks fo your awesome feedback :smile:

[yanickrochon]

Reading the roadmap, being farily new to React and all, I found this issue and wondered why this.refs was being recommended? Isn't this ignoring React's own warning about Don't Overuse Refs?

[oliviertassinari]

why this.refs was being recommended?

@yanickrochon This thread is almost 1-year-old. Things have changed in the ecosystem since then.
That's why.
One interesting approach is to allow parent having access to the children is by adding a rootRef callback property, E.g.

[oliviertassinari]

I have come to the conclusion that state is bad for business.

@alitaheri I 100% agree, we have removed as much as state as possible in the components migrated to the v1-alpha branch. I think that we can close it, we have learned from our mistakes :).