Baseweb: [Proposal] Change handler consistency

[schnerd]

Here's a quick snapshot of our change handlers:

Checkbox: onChange(evt) { console.log(evt.target.checked); }
Input: onChange(evt) { console.log(evt.target.value); }
Menu: onItemSelect(selectedItem: object)
Modal: onClose(closeSource: string)
Pagination: onPageChange?: (nextPage: number, prevPage: number)
Popover: onOpen(), onClose()
RadioGroup: onChange(evt) { console.log(evt.target.value); }
Select: onChange(evt, params) { console.log(params.selectedOptions) }
Slider: onChange(evt, value) { console.log(value) }
TextArea: onChange(evt) { console.log(evt.target.value); }

As you can see, there's a pretty significant amount of variation between components. It would probably be good for us to make these more consistent, or at least develop some guidelines around what to use when.

I'd like to make a case for gravitating towards the following API long term:

onChange(value, event, meta)

• If this is a form control with a value, just pass that directly as the first argument. This prevents people from needing to remember how to dig into the event object like evt.target.checked or evt.target.value, etc. It also works better for components like multiple Select or Slider where the event doesn't actually have the form control value.
• Event is second. I'd argue if you have the value first, event is rarely needed. It can also be confusing for something like multiple Select, where things like removing a tag technically trigger a change event. Or for pagination where clicking back/next or selecting a page from the dropdown can both trigger page changes.
• Meta is third. This is an object that can include things like previous value (if useful), or any other metadata that may be useful to the user.

The only benefit I can think for keeping event as the first argument is that it matches what people expect when using a native html element, but in many cases our components are not native html elements and the event can come from multiple different types of dom interactions. I'd rather be consistent within our own components than consistent with native elements.

Curious to hear what folks think about this – do you agree there's a need to standardize? Are there downsides with the api proposed in this issue?

[gergelyke]

Thanks for putting this together David!

As I see, moving to a standardized change handler makes users' life a lot simpler. The only thing I'd change is the order of the arguments to be the following: onChange(event, value, meta), because of:

• more intuitive - if you write native change handlers, you'll get the event as the first too
• more widespread - examples include materiual ui, gestalt or garden

While I agree that in most cases we'd only use the value, being consistent with native apis and other component libraries outweighs the cons for me.

[schnerd]

more intuitive - if you write native change handlers, you'll get the event as the first too

I agree it's more similar to native, however in my opinion the frustration users would experience when seeing the inconsistency of event objects outweighs the desire align with native. Our <Select>, for example, does not emit native select change events and if a user tried to access event.target.value it may not be available, this could mislead and confuse many users. Similarly, the events emitted by Pagination/Slider/Datepicker/etc components will not have any information related to the value. I would not describe these cases as "intuitive", on the contrary I think passing the value first is much more intuitive, even if it doesn't match the native APIs.

more widespread - examples include materiual ui, gestalt or garden

I'm not sure it's actually that consistent among libraries. evergreen, atlaskit, react-select, react-dates, and others seem to use value-first. Since there's not widely accepted standard, at the end of the day we should probably do what is easiest for users.

[gergelyke]

makes sense David, thanks for the extra information 👍 - it seems I've only checked the libraries which reinforced my point :facepalm:

[abmai]

Maybe controversial opinion here. If we're building for the future anyway - and we're not entirely trying to mimic native handlers, why not go all the way to object params i.e. onChange({value, event, meta}) so we can forgo order altogether and allow users to just pick out what they need?

I agree that we should come to some sort of standardized API for the non-native change handlers.

[gergelyke]

i like this approach a lot @abmai

[schnerd]

That's a big improvement over event-first I think. With this paradigm we may not need the explicit meta property anymore–those properties could just be added to the main object onChange({prevValue}).

I do wonder what this object should be named if its not being destructured. Maybe we could use meta for that? onChange(meta)

[abmai]

What about data?

[blackswanny]

In my recent designed components value was not popular, or it was not a primitive type (object for instance). So I would leave event as first argument. Sometimes value is found in event itself. So my preference to leave only two arguments: event and meta, which may contain any extra information helpful for consumption, including the value itself

[schnerd]

In my recent designed components value was not popular, or it was not a primitive type (object for instance). So I would leave event as first argument. Sometimes value is found in event itself.

By the same reasoning, sometimes value is not found in the event (select, slider, pagination, datepicker, etc), so event is "not popular" in those cases. By encouraging users to use the explicit value property we'd reduce the cognitive overload they have when implementing different components.

I think Anh's API does a pretty good job of balancing the concern of whether event or value should come first – they're both available if necessary, it's consistent across all components, and the value is readily available.

[blackswanny]

@schnerd what should we do if there is no value for onChange handler (or even other handlers)? Set it to undefined ?

[schnerd]

Just leave it out. Modal onClose for example: onClose({event, source})

[nadiia]

What about data?

How about still calling it an event but it considered a modified one, and the prop name for the native event would be originalEvent?

I like a single object type argument since it's more flexible and expandable.

[gergelyke]

What would be the benefit of calling it data and what would be the benefit of calling it event?

[schnerd]

Calling it event makes sense semantically, but when reading code/docs people might initially think it's the normal event object until they read further – it seems like a different name would reduce ambiguity.

Also originalEvent might be the react synthetic event, if users want the real native event for some reason they might have to do event.originalEvent.nativeEvent which feels a bit gnarly.

Oleg called it params in Select, which could be good. It's a tossup for me between that and data.

[gergelyke]

That makes sense - @nadiia would you be ok with data or params?

[abmai]

In terms of follow-ups (once we determine the final object name - I don't really have a preference there), looks like this will be a breaking change.

There are some other breaking changes that I'd like to get in as well for Menu. Should we plan for a v2.0.0, and gather API change proposals somewhere?

[gergelyke]

Just created a 2.0.0 milestone - just add any issues/prs to it that you'd like to include

https://github.com/uber-web/baseui/milestones

[blackswanny]

So I am for one argument as well. Imo, more than 2 arguments becomes confusing of purpose.
One param arg is more flexible.

[gergelyke]

@schnerd @abmai @blackswanny actually, do we want to have a 2.0.0 milestone?

How about we just do major bumps for this, and for the menu too? Then we wouldn't have to wait for merging PRs, so we could move faster with addressing these changes. The user of this library would have to update their usage either way, just that if we release a major whenever we are ready, they can do it in smaller batches. Also, for now the adoption is small, so that's not something we should be worried about neither.

What do you think?

[blackswanny]

@gergelyke Ok. nit: usually next major version means some complete change in framework.

[gergelyke]

strictly speaking, it just means an incompatible API change

[abmai]

Hm, maybe we can still do a "milestone" release, but apply some sort of cadence to major version bumps, so we're not just bumping major versions all the time?

My worry is that a lot of the components are still being battle-tested, whether through our own integration or not, and we might need to evolve the API a handful of times in the next few months. How do we make sure we don't end up with baseui v5.0.0 by end of the year? Or is that not really something we should be concerned with (which could be a totally valid approach)?

Thoughts?

[gergelyke]

I wouldn't be concerned with that - as I see that's just a number. The smaller the API change for any given major version, the simplest it will be for consumers to upgrade.

What's your take on it @schnerd @nadiia ?

[abmai]

Just discussed with @gergelyke in chat, we agree that since we're still under pretty heavy development, we should optimize to move faster and bump major versions as often as needed. Once we are more stable (and baseui is used by more internal apps), we can slow down to a better major release train. What does everyone else think?

[blackswanny]

all good. Just in case if anyone used our baseui already one of the ways is to do a pre-release version (npm suffixed), so they won't update it automatically and wont take breaking change, send release email and only after remove pre-release suffix from version

[abmai]

all good. Just in case if anyone used our baseui already one of the ways is to do a pre-release version (npm suffixed), so they won't update it automatically and wont take breaking change, send release email and only after remove pre-release suffix from version

Major bumps should already prevent these accidental upgrades for existing users so I don't think we need to do a pre-release. What other cases were you thinking of?

[blackswanny]

no other. What is major bump in this case? we change signature of handlers, so user has to change usage of handlers as well

[gergelyke]

@schnerd do you want to take this on, or do you want us to prio it for one of the next sprints?

[schnerd]

Yeah I can take it on. For any component that hasn't been implemented/landed yet, let's switch to this new style? I can update all the components that are already on master.

[gergelyke]

that sounds good, thanks!

[gergelyke]

It was released in 4.0.0