@lit/react binds event callback prop as instance property, overriding the element's class method

[stefanpearson]

Which package(s) are affected?

React (@lit/react)

Description

This one gave me such a headache, it took me ages to figure out what was happening here!

Initially, I thought this was some synthetic event pain, but discovered that when wrapping an element in the React component factory, it seems the react prop is overriding the actual class method. In my case, an onInput prop is overriding the internal onInput class method.

React app:

import { createComponent } from "@lit/react";

const SearchField = createComponent({
  tagName: SearchFieldElement.tagName,
  elementClass: SearchFieldElement,
  react: React,
  events: {
    onInput: "input"
  }
});

const MyComponent = () => {
  const onSearchInput = useCallback(
    (event: React.FormEvent<SearchFieldElement>) => {
      // this is binding straight to the Lit render template!
    },
    []
  );

  return (
    <SearchField onInput={onSearchInput} />
  )
};

SearchField class:

class SearchField extends LitElement {
  static tagName = 'my-search-input' as const;

  protected onInput(event: InputEvent) {
    // This is overridden by the onSearchInput prop reference 
  }

  render() {
    return html`
      <input @input=${this.onInput} />
    `
  }
}

Reproduction

See code example, I can create a sandbox if necessary

Workaround

Rename the internal class method name 🤷

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

"@lit/react": "1.0.3", "lit": "3.1.2"

Browser/OS/Node environment

Seems to be all browsers

[augustjk]

Thank you for the report!
This specifically happens when the React component is re-rendered but given the same value to the mapped event handler prop as you do by using useCallback().

I found the source of the bug here

lit/packages/react/src/create-component.ts

Lines 168 to 171 in 1b00c07

	if (event !== undefined && value !== old) {
	addOrUpdateEventListener(node, event, value as (e?: Event) => void);
	return;
	}

We are not early returning if a re-render happens with the same reference on the event binding, which ends up setting it on the instance. I'll have a fix up soon.

As an aside, I still like prefixing internal methods with _ if not using private fields with # which makes it so it wouldn't be overridden. Also on prefixing is often used for public interfaces so I like prefixing internal ones with handle, like handleInput rather than onInput.

[stefanpearson]

Thanks @augustjk! This would also explain why it worked as expected once and then falls over afterwards!

[stefanpearson]

FYI @augustjk, I've changed all internal private callbacks to be prefixed with _. However, it makes me nervous that, unless specified in the events map, any innocent internal handler (e.g. onKeyDown) could simply be overridden by a React prop. As TS didn't catch this issue, I ended up having to create a global test that checks an instance whether it has any 'reserved' prop names, which doesn't feel right.

/* Taken from DOMAttributes type in React */
export const REACT_EVENT_CALLBACK_NAMES = [
  "onCopy",
  "onCopyCapture",
  "onCut",
  "onCutCapture",
  "onPaste",
  // etc
] as const;

export const runReactSanitizeCheck = (Element: CustomElementConstructor) => {
  /**
   * When using `@lit/react`'s `createComponent` to create a React component, it unfortunately
   * just assigns any property that is part of React's props interface. This means that when
   * a consumer uses a React component like `<CustomElement onClick={} />`, the `onClick` property
   * will be set on the instance, and override any prototype property.
   *
   * This can be problematic and can lead to obscure issues that are difficult to track down,
   * without any TypeScript errors, or runtime errors. It will simply override the private handler
   * and lead to unexpected runtime behavior.
   *
   * Until React really fixes its interface with custom elements (React 19 may solve this?),
   * or `@lit/react` ensures this does not happen, we have to forbid any keys on an element's
   * prototype that could conflict with these React event callback names.
   */
  it("does not use any prototype properties that could be overridden by React", () => {
    const elementInstance = new Element();

    REACT_EVENT_CALLBACK_NAMES.forEach((callbackName) => {
      assert.isUndefined(
        (elementInstance as any)[callbackName],
        `Element should not use ${callbackName} as a prototype property, to avoid being overridden by React props.`
      );
    });
  });
};

Is this a case of a gotcha to look out for, or could @lit/react make an effort to guard this, as it currently does with ref/children/className etc?

If this is something that you agree with, I'm happy to take a look at making a PR at some point!

[augustjk]

Ah, that's a valid point. I can indeed confirm that "internal" methods named that way does prevent those React built-in event handlers from working. I think this warrants a separate issue and discussion. I made one here #4585