Standardization of Callback Signatures
alitaheri opened this issue · 30 comments
We need to discuss how will we standardize the callbacks that some component take in order to behave in a controllable fashion. But since the signature of the callback function they accept is different across components, many new comers get confused.
Therefore, we have to come to a solution on this.
My proposal:
- Every controllable component should only be controlled via
valueandonChangeprops. No moreonRequestXxx. - the
valuemust be as versatile as possible, if it make sense for it to be anything equatable with===then it should be marked as any. - The value should NOT be treated as
falsyEVER! Implicit coercion leads to many many bugs. - The signature of the callback must follow this pattern:
(e: Event, v: value, ...rest: any[]) => void - If access to the event is granted, there really shouldn't be any reason to provide a
reasonargument to indicate what triggered the change. The event can be checked against it's type or it's properties. I think this could be more accurate. (debatable 😁 )
@oliviertassinari @newoga @mbrookes Tell me what you guys think 😁
The Roadmap:
1. Standardize The Following Components:
-
DatePicker: onChange(event, date) -
DropDownMenu onChange(event, key, payload) => onChange(event, value, index), index? -
IconMenu (event, value) -
Drawer (deprecated prop) onChange(event, key, payload) => onChange(event, state) -
RadioButtonGroup onChange(event, newSelection) -
SelectField, DropDownMenu onChange(event, index, value) => onChange(event, value, index) -
Slider onChange(event, value) -
Tabs onChange(value) => onChange(event, value)#7741 -
TextField onChange(event) => onChange(event, value)#3699 -
TimePicker onChange(null, time) => onChange(event, time) -
Table(#3002)
2. Purge State:
- Create a stateful
StateWrappercomponents that can be used to make components stateful, useful for forms. It must implement:getValue(),setValue(value),clearValue(),defaultValue prop,onChange prop: (event, value, ...rest) => void - Clear state from all components that can be stateless. As much as possible.
3. Documentation and deployment:
- Figure out a way to make usage less pain full, something like:
import SelectField from 'material-ui/lib/SelectField'; // For the actual component
// As opposed to:
import SelectField from 'material-ui/lib/SelectField/stateful'; // For the stateful version componentThis is easy to implement, but I don't know if we want this, we should discuss this too.
Thanks for making this issue! 👍 I agree and am really looking forward to when this is done across the library. This topic is definitely one of my pain points with integrating material-ui into one of my projects. I'm sure this would make @mbrookes' library a good deal simpler too 😄
I haven't fully considered the implications of all your points (including point 5) but I definitely agree with this direction. I will continue to give it thought.
I would be curious to hear your opinion regarding something like our AutoComplete component. Many libraries have a separate event to distinguish when the user is changing the input (such as onInputChange) vs actually changing the value (such as selecting an item from a list of possible value would call onChange). If we used that approach, we would break these rules a bit, but I'm open to new ideas for this too.
If access to the event is granted, there really shouldn't be any reason to provide a reason argument to indicate what triggered the change.
Does this mean you would recommend calling onChange for both types of events and passing a different type of event for when the input change vs the value changes?
Of all the proposed core changes, this one may be the most painful, as it doesn't naturally allow for to deprecation warnings.
One option may be to couple this with the component renaming to PascalCase, so that the old import returns the old callback signature, and vice-versa, and the deprecation warning for the old component name notes that the signature has changed in the new component.
@newoga The only way we can know the reason is through the kind of event that was fired. events have pretty good API and support instanceof checks. so it can be implemented if it would matter that much. But I'm no expert on this front. I will change that rule if I'm wrong.
If we used that approach, we would break these rules a bit, but I'm open to new ideas for this too.
These have nothing to do with controllable behavior. These are different aspects. I just meant that for controlling a component we should have different naming conventions like onRequestClose or onRequestChange or onRequestOpen or etc. I mean they should all be renamed to onChange
This way, we can get rid of uncontrollable behavior with implementation of one single wrapper that understands this single standard: value + onChange(event, value).
@mbrookes It will be painful for only a couple of components if we actually rename the props too. only a few of them are named onChange.
Well, that's a good point!
@mbrookes Great idea 👍 👍
But I think we better not make any more changes before the 0.15.0 release. the changes are so many as it is, I'd postpone it for the 0.16.0 release. Except maybe for the components that don't have onChange, so it would we a feature addition, not breaking change 😁
Really? Looking at the roadmap, I'm not seeing much in the way of true breaking changes. If users have been heeding the deprecation warnings, there shouldn't be any nasty surprises. Or is there more to come that's not listed there (but should be)?
And to your point:
It will be painful for only a couple of components
And I'm sure somewhere else we agreed sooner is better than later for breaking changes, as the more people use the library, the more are affected by a later breaking change.
Just saying. 😁
Really? Looking at the roadmap, I'm not seeing much in the way of true breaking changes. If users have been heeding the deprecation warnings, there shouldn't be any nasty surprises. Or is there more to come that's not listed there (but should be)?
I guess you have a point 😅, I think it would be a good idea to introduce some deprecations with the 0.15.0 release so we can make these standard. I'm just very afraid of callbacks. we can't warn users. we can't do any thing, and the effect might go unnoticed for a long time. we should heavily document the transition.
And I'm sure somewhere else we agreed sooner is better than later for breaking changes, as the more people use the library, the more are affected by a later breaking change.
Yeah, that's scary O.o
callbacks. we can't warn users. we can't do any thing
Well, sure, but that's what makes it a breaking change that had to wait for a 0.x release.
I agree it's going to need a big-ass warning all over the place though! As well as the README and docs, we could have npm spit out a big warning using a postinstall script.
Let me have a run through and see which components already have an onChange prop, and which ones would require their params to be reordered. Anything else is just a new onChange prop, or a new deprecation warning (and those could wait be be introduced in a 0.15.x release to be removed in 0.16.0)
Okay, here's what I found ComponentName (onChange params):
DatePicker (event, date)
DropDownIcon (event, key, payload)
DropDownMenu (event, key, payload)
IconMenu (event, value)
LeftNav (deprecated prop) (event, key, payload)
RadioButtonGroup (event, newSelection) < RadioButton onCheck(event, value)
SelectField < DropDownMenu (event, index, value)
Slider (event, value)
Tabs (value)
TextField (event)
TimePicker (null, time)
Internal naming varies, but if we assume null = event, key = index, and payload | newSelection = value, in which case:
(value):
Tabs
(event):
TextField
(event, value):
DatePicker
IconMenu
RadioButtonGroup < RadioButton onCheck
Slider
TimePicker
(event, index, value):
DropDownIcon
DropDownMenu
LeftNav (deprecated prop)
SelectField < DropDownMenu
Wow, thanks a lot 👍 So we'll need only 3 breaking changes that we can't warn users about. For Tabs, DropDownMenu, SelectField. since LeftNav will have it's onChange removed, we can just use it for that purpose.
DropDownIcon
is being removed 😁
I'll update the issue description. 👍 Thanks a lot @mbrookes
@newoga @oliviertassinari @mbrookes Please take another look at the title please 😁
@alitaheri I think I'm good moving forward with at least point 1 in the issue description. Maybe we could tackle the non-breaking changes components together as a PR and the breaking changes components individually in their own PRs so we can look more carefully, but otherwise I'm happy 👍
I agree with the general approach to moving toward stateless but haven't fully considered the design mentioned in points 2 and 3, but we can discuss that more in the future.
@alitaheri thanks for taking my crib notes and turning it into a meaningful proposal!
Just wanted to point out I only listed instances of onChange, as these were the ones we might have to reorder props as a breaking change.
To completely standardise the API, (step 1a? 😄 ) there are also the components that use something other than onChange (such as onCheck in Checkbox and RadioButton, onToggle of Toggle, etc.)
Those will be much less painful, as we can introduce onChange and deprecate the the old prop with a warning.
@oliviertassinari Can 2 and 3 be moved to another release?
Yeah, that sourds like a good idea. Let's reduce the scope of this big upcoming release.
Have you considered adding flow to mui to guarantee this and aid in refactoring? I've got it setup in my new project and it has already helped a ton. Given that it is opt-in by default, we can add it class by class. I think it is something to consider given the size of the project. It should ease refactoring and maintainability, as well as integrate into CI to validate PRs.
@rosskevin I personally prefer having a type system of any kind. But the issue is, not everyone know them. That means besides learning about our huge codebase, new contributors will also know about flow. That's not very good for big community-driven projects like this I'm afraid.
Since it is opt-in, by the time they PR a class change there should be plenty of examples. Additionally, a PR can be submitted failing flow and we can help. I disagree that it doesn't apply. With the size, complexity, and many contributors, it is needed here more than most
You described the proposal in flow terms; I think using flow is a critical step for maintainability.
That's not very good for big community-driven projects like this I'm afraid.
@alitaheri I agree, but can't we use flow without adding any type informations?
I think that flow can extract some informations by just parsing the AST and looking at the associations.
I'm wondering how smart he is.
@rosskevin I'm more of a typescript guy, I don't know flow myself 😅 As I said, I myself prefer a type system. But only if it's done in a way that doesn't slow down new comers.
@oliviertassinari I haven't used flow in any of my projects, but if it's from the react guys, it should be smarter than most for react components.
Flow and typescript are similar and on-par with each other (and often look very much the same). One nice thing about flow + react is that we can type PropTypes and State, plus we can use that in place of using React.PropTypes. This will give us compile-time type checking. It will also break when we pass state or props that don't line up with the definition. Another thing about typed systems, similar to how proptypes are done (but shorter/more elegant) they are self documenting.
Many errors are found at compile time that you would normally miss in testing (or have to dramatically expand your test suite).
One more note: you can add flow and wait to enforce it - it will have no affect on devs or users as it strips out the definitions at transpile time.
@rosskevin Those are some decent points you've made. I don't disagree with any of them. Might not be a bad idea to open a proposal issue for it so we can get feedback from the community 👍
what is the progress on this?
I have a generic change data handler, which changes the underlying data based on the id of the component that fires the onChange event. I would like to be able to set the id of each of the material-ui components and for that id to either be set in the change event's event.target.id or for the id parameter to be standardised as a parameter in the onChange events (prefered). Note this is different to the key parameter which is used where options are involved. At the moment I am forced to pre-bind the id onto my event handlers.