Skip to content

Fix dispatch wrapping for internal events #1283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgmolisani
Copy link

What

There a few places in the library where events are dispatched in response to already triggered events. These seemingly fix missing/differing functionality in JSDOM for events relating to focus. This includes handling focus/blur when visibilityState is hidden and triggering change events on blur. However, these are also wrapped in the runtime's configured eventWrapper. In React, this is the act wrapper. Due to how React flushes the update queue in act environments, this can prematurely tear down the queue making future updates trigger...

Warning: An update to %s inside a test was not wrapped in act(...).

Users cannot avoid these error logs (and in our case, we elevate them and fail our tests as if they were real errors). This can be completely avoided though as when we trigger events for focus and blur, we should already be in a wrapped scope. To further that point, imagine in user space we had a component that re-dispatches a custom change event:

function Component(): ReactElement {
  const handleChange = (event: ChangeEvent<HTMLInputElement>): void => {
    const customEvent = new CustomEvent("custom", {
      detail: event.target.value,
    });

    event.target.dispatchEvent(customEvent);
  };

  return <input data-testid="input" onChange={handleChange} />;
}

In this case, it's obvious that we cannot wrap our dispatchEvent call in act as it's part of the component yet this is totally fine because our synthetic React change event presumably was triggered by a wrapped call like userEvent.type(screen.getByTestId("input"), "some text"). The same should be true here where the focus/blur events are already wrapped and the wrapping around the patched event is unnecessary (and, in the case of React, problematic).

Why

In React, nested synchronous act calls can prematurely teardown the act queue creating console warnings. In my team's case, we elevate these to real errors in our tests making them more visible. These warnings can be avoided as described in the What section. It's also possible React could warn about act wrapped state updates on the queue and might even handle the nesting in async cases already. Nesting seems like it should be avoided though as users can't really create the same behavior in a real app. It's still not totally clear to me why this is allowed or if it should be handled already by React and is a bug. But in any case, I don't think we have to wrap the internal calls and can solve here.

How

I removed the call to wrapEvent in dispatchDOMEvent which is currently only used for these focus/blur event. I am not sure this was the original intention for the split dispatch compared to dispatchEvent in the same file, but it seems very fitting as fixing here fixes all the current cases where wrapping should not be added and doesn't touch the places wrapping is required. I also passed through the boolean return value. It's not used anywhere but seemed like it fit the intention of the function to just be a simple wrapper around the native dispatchEvent API.

Checklist

  • Documentation - Internal, no updates required.
  • Tests - None needed to be updated (which is a good sign).
  • Ready to be merged - No one should be relying on this behavior as is.

#1275 - Another user reported this issue while also introducing a new feature. I believe the solution in this PR is more universal and decouples from the feature request to hopefully move this along more quickly.

Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant