Polaris-react: Should Events expose DOM Event parameters in their typings?

[BPScott]

Currently Events such as onClick, onFocus and onBlur on various components - mostly Link and Button which in turn derive from UnstyledLink - do not expose their Synthetic Events as available arguments within the typescript typings, even though those arguments are present at runtime.

Consider the following Playground:

/* eslint-disable */

import * as React from 'react';
import {AppProvider, Button} from '@shopify/polaris';

export default class Playground extends React.Component {
  click() {
    console.log(arguments[0]);
  }

  render() {
    return (
      <AppProvider>
        <Button onClick={this.click}>A NORMAL BUTTON</Button>
      </AppProvider>
    );
  }
}

When clicking the button the Synthetic Click event is logged. However I can not rewrite the click handler to as click(e) { console.log(e); } as TypeScript claims the function does not accept any arguments.

This was originally done as we wanted to avoid exposing events as we wanted to encourage people to do things the reacty way. (Thanks @dfmcphee for context). However there are some cases where this event is useful, for example a Button with a default URL and an onClick event, where the onClick wants to call e.preventDefault() to stop the link to that page. @dfmcphee says this issue has arisen in the past too.

Here's a question: Why do we consider people using the event argument not idiomatic?

I see a few directions:

---

One: Leave things as is - Synthetic events are not exposed in the typings.

This stops people digging into event hooks even when they have a legitimate need to, unless they do ugly stuff with arguments[0] which is not type-safe and if I saw it I'd think it was a smell and I'd be amazed that it worked.

Pros: Heavy encapsulation. Shows people that the vast majority of the time they shouldn't need to touch events.
Cons: Blocks occasional, rare, legitimate use.

---

Two: We add the Synthetic Event argument to onClick, onFocus, onBlur etc arguments for Buttons, Links, UnstyledLinks etc.

Pros: Our types directly express what is available to consuming code.
Cons: No forceful way to express that using events is discouraged the majority of the time. Only within documentation.

---

Three: @a-mooz suggested a mechanism where the presence of an additional attribute can change the typing to expose the event by using some intersection type shenanigans. E.g.

export default class Playground extends React.Component {
  click(e) {
    console.log('I would not compile, the e argument needs to be removed');
  }

  clickAlt(e) {
    console.log('I would compile', e);
  }

  render() {
    return (
      <AppProvider>
        <Button onClick={this.click}>A NORMAL BUTTON</Button>
        <Button canAccessEvent onClick={this.clickAlt}>
          {' '}
          A NORMAL BUTTON
        </Button>
      </AppProvider>
    );
  }
}

Pros: Allows us to hint that accessing the event isn't the default
Cons: Additional complexity, people need another prop

[amooz]

Option 3 is similar to reacts dangerouslysetinnerhtml flag. It doesn’t stop you from doing a thing, but it will make you work a bit harder to do it.

https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml

[dfmcphee]

cc @lemonmade since you were involved with the original decision

[lemonmade]

There is a fourth option, which is to find the reasons people want access to the underlying events and add props/ more specific arguments to tailor to those cases. For example, if we feel preventing navigation when there is a URL + on click is not a sensible default, but is sometimes desirable, then you could add a prop to unstyled link/ button / link to do this. This is basically in line with the form components where people also asked for the event — we ended up just giving the ID because most people needed it to support a single callback for all form fields that updated some state based on a unique identifier for the field.

Now, whether that game of whack a mole is actually the right call is one I can't really make, I don't have all the info needed. I do think it is technically purer and, if you choose to abandon it and give raw access to the events, you have to be ready for the fact that you have now opened up a whole lot of use cases that effectively have first-class support in the API.

[calumptrck]

Hey @BPScott @lemonmade has there been any further discussion on this issue?

I have a case where I'm using the Link component nested inside a DropZone. I want to call e.stopPropagation() inside the Link onClick handler so that clicking the link doesn't trigger the DropZone, but I'm blocked by this issue.

I'm interested in working on a PR for this if there is an agreed upon option.

Thanks!

edit: Found a workaround for my particular use case.

[sijad]

@calumptrck I have exact problem, I need to use a button inside DropZone, can you please share your workaround?

[sijad]

for now I've wrapped my Button around a <div> with an onClick:

const handleOnRemove = useCallback(
  (e: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
    e.stopPropagation();
    onRemove(id);
  },
  [id, onRemove],
);
// ...
return (
  <div onClick={handleOnRemove}>
    <Button destructive icon={DeleteMinor} />
  </div>
);

[calumptrck]

Hey @sijad, sorry for the delay. I'm no longer an intern at Shopify so I can't take a look at exactly what I did, but I believe my solution was to place my Link adjacent to the DropZone rather than nesting it inside, and absolutely positioning the Link relative to the parent. Your solution looks good as well but it will lose some of the document semantics and accessibility by putting the click handler on the div rather than the button.

I think ideally there should be a prop on links/buttons to make this easier as Chris said above. Hope this helps!

[sijad]

I have another usecase with formik v2.

useField returns an object with an onChange function that you can pass it to your form component as props and it handle all of field changes, but as Polaris does not pass native event to onChange we can not use it and we have to recreate it to make it compatible with polaris.

[felipeleusin]

There is a less ugly way to use this that is passing the event as an optional argument and just checking for presence:

/* eslint-disable */

import * as React from 'react';
import {AppProvider, Button} from '@shopify/polaris';

export default class Playground extends React.Component {
  click(ev?: React.MouseEvent) {
    if (!ev) return;
    console.log(ev.target)
  }

  render() {
    return (
      <AppProvider>
        <Button onClick={this.click}>A NORMAL BUTTON</Button>
      </AppProvider>
    );
  }
}

I feel like exposing the synthetic events doesn't hurt in this situation and prevents the teams using Polaris to have to implement workarounds and hacks and there are a lot of legitimate reasons to access the synthetic event. I was wondering why is accessing events discouraged?

We could try to map all the needed things with props but it ends up just creating more complexity. We risk ending up with a button that has props like onClickPreventsDefault={true} onClickStopsProppagation={true} or who knows what

[lemonmade]

The reason to avoid these events is more important now than it ever has been for us. The DOM is an implementation detail of our applications. Many of our apps will need to start targeting layers that are abstract enough to run in the DOM and our native apps as we try to expose app functionality in more areas. If we tie all our apps to the DOM, we will need to rewrite them all to take advantage of these abilities.

The DOM is not a great API for building apps. If we have information that makes sense to communicate back in a Button’s onClick (maybe the ID you pass to the button), align on those APIs and build them. DOM events have more API than we can possibly want to promise, since they give you access to the raw DOM nodes.

[felipeleusin]

So what APIs would be?

From what I saw on this issue and on email we need:

1. stopPropagation, preventDefault
2. button / metaKey / controlKey / altKey

The first two can be shimmed as noops in an unsupported environment and the others can be just false.

[lemonmade]

@felipeleusin I think you should open an issue for the things you can't do with the current API.

[probot-shopify[bot]]

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.