Audit components: event callbacks should pass syntheticEvent, then customData object

[donnieberg]

Suggestion from @tweettypography

Make all custom event callbacks have two properties handleEvent(customData, syntheticEvent). The first prop would be an object with whatever is being passed back from the app - updated values, etc - and the second is simply the original triggering event.

TO DOS

• Change all components to this pattern
• Update docs

[tweettypography]

For events that have an existing React syntheticEvent it likely makes more sense to reverse the two properties but many callbacks we use don't have an original event, which would leave the first property null. I'd be interested in hearing thoughts on this.

[tweettypography]

I've started on this issue in this branch: https://github.com/salesforce-ux/design-system-react/tree/standardize-events

The path I'm following in that branch is to have only a single event argument with any additional data required added as properties to that object. If there is a valid syntheticEvent to use as the argument the properties are added to that event. If there isn't, they'll simply be added to a plain object. The end result being handleEvent(syntheticEventWithCustomData)

[tweettypography]

ContextBarLink is currently being updated to this pattern in the following PR: #363

I'll finish up the rest of the components and do a PR for them later, but this will obviously be a quite large breaking change so it might take some coordination before we actually release it.

[tweettypography]

After some consideration, I could see some valid reasons not to add the custom props to the existing event object. For example, there could be conflicts in property names, React recycles its event objects which may not be desirable, and in situations where an event doesn't exist it could be confusing that expected methods like preventDefault are not present. A better alternative may be to leave the event as the first argument and pass the custom properties as a second object, and when an event is not available but it's an event-like callback pass a null as the first argument. When there's no event and the callback is not event-like simply pass the custom properties as the first argument.

No matter which solution is determined to be best, we also need to standardize on the names of custom properties that we will return and keep the list as short as possible.

[interactivellama]

An additional reason to not update the event object is that React expects it to be immutable and complains if it is not.

Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're adding a new property in the synthetic event object. The property is never released. See https://fb.me/react-event-pooling for more information.

[interactivellama]

TL:DR;
Looks like the current path forward is to use eventCallback(event, { key: value }) {} pattern.