Skip to content
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

[React@18 in legacy mode][EuiMemoryTable/EuiSearchBar] Skipped letters while typing #8018

Open
Dosant opened this issue Sep 12, 2024 · 7 comments

Comments

@Dosant
Copy link

Dosant commented Sep 12, 2024

Hi Team! We in Kibana have started working on upgrading to React@18 elastic/kibana#138222 (comment). We hope that we can do a gradual upgrade where we first upgrade Kibana react packages to React@18 version, and run Kibana with these packages with ReactDOM.render legacy mode, and allow teams to upgrade to createRoot runtime gradually.

Unfortunately, we're now observing some runtime changes in React@17 -> React@18 in legacy mode. We're not sure yet how bad the changes are and if we continue with this gradual migration plan. The changes don't seem to be properly documented by the React team. But our functional tests discovered some issues and a very common one is with EuiTable/EuiMemoryTable and EuiSearchBar integration.

It looks like React@18 in legacy mode discovered some problems there with state management that I hope we could look at.

The problem in our tests shows up as if some of the keys from the keyboard input are skipped when typing into the EuiSearchBar

Should be my-favorite but typed my-fvoir

Image

I tried to reproduce in sandbox: https://codesandbox.io/p/sandbox/fast-fast-6hvctr?file=%2Fdemo.tsx
If you play with it and type with different speed, you'll see that the input has problems and sometimes the input is skipped

See I typed anton but only aton was registered by the input

Image

We've spent quite a bit of time debugging, and I pinpointed the problem to this state management place:

useUpdateEffect(() => {
if (inputRef.current) {
inputRef.current.value = query;
inputRef.current.dispatchEvent(new Event('change'));
}
}, [query]);

What happens is when I type "Anton" " after typing the "A" "n" the component could re-render, and the effect is triggered with query a and overrides the an in the input with a from the query

I don't know the details of what has changed in React internals and why this could happen now but not before, but our tests reliably fail due to this in a lot of places where tables and search bars are used. I wonder if some of you would have more experience and insight to understand why this breaks.

I understand that running EUI with React@18 in legacy mode was probably not something we'd explicitly test or support, but I'd like to ask for some help looking deeper into this and maybe addressing this particular place. It might be that we'd drop this legacy mode idea if we'd see more issues like this, but for now would like to try to address this

@Dosant
Copy link
Author

Dosant commented Sep 13, 2024

I found that this particular problem is fixed if I change useEffect -> useLayout effect here, but I can't explain why

useUpdateEffect(() => {
if (inputRef.current) {
inputRef.current.value = query;
inputRef.current.dispatchEvent(new Event('change'));
}
}, [query]);

From the list of breaking changes list in React@18 this change stands out:

Consistent useEffect timing: React now always synchronously flushes effect functions if the update was triggered during a discrete user input event such as a click or a keydown event. Previously, the behavior wasn’t always predictable or consistent.

but it is unclear if this change applies to legacy mode and I am not 100% sure this caused the bug for us. The explanation is very vague and I couldn't find more details or examples

@Dosant
Copy link
Author

Dosant commented Sep 13, 2024

I found a very simple way to reliably reproduce this problem

See how the letters are lost.

@cee-chen
Copy link
Member

Apologies for not responding to this earlier Anton, this slipped through the cracks for us.

It looks like you have a workaround with useLayoutEffect that we would 100% accept a PR for, or open one on your behalf, if that's easier for you. I'm unfortunately not at all familiar with React 18 in legacy mode so I'm not sure how much help I could be with investigating the "why" of it.

@tkajtoch led the React 18 upgrade for EUI and could likely help out more, but is out this week for PTO - if he doesn't respond at that point, would you mind reaching out to Tomasz?

@Dosant
Copy link
Author

Dosant commented Sep 19, 2024

@cee-chen, thanks for looking.

This is not the only issue we're seeing in Kibana when running react@18 in legacy mode. We are not fully sure yet, but because of hidden breaking changes like this, there might be no sense in doing the update to legacy mode first, since we will have to fix and test everything twice.

But if someone with React experience could take a look at the issue and could help us better understand what is causing it, this would help, as we might track down more places like this. We're thinking that maybe it is worth creating an issue with react team, but I couldn't create a simple sandbox to reproduce

@Dosant
Copy link
Author

Dosant commented Sep 19, 2024

I was finally able to reproduce the problem without EUI. I was copying the unusual patterns that EUISearchBar > SearchBox > SearchField was using and then trimming everything that was not needed to see the problem: https://codesandbox.io/p/sandbox/minimal-report-react-18-legacy-forked-27c5qj?workspaceId=05eaf057-3cbb-4e04-a1ab-b97be769b1bc .

  • This works with React@17
  • This doesn't work with React@18 in Legacy
  • This works with React@18

The pattern

useEffect(() => {
    if (inputRef.current) {
      inputRef.current.value = value;
    }
  }, [value]);

Seems like a code smell and seems like shouldn't have reliably worked in the first place. But surprisingly it breaks only for React@18 in legacy. I wonder if it is worth creating an issue with React team as this is only broken in React@18 legacy

@cee-chen
Copy link
Member

++ to creating an issue width the React team personally, it seems incredibly odd that it only stops working in legacy mode.

As a heads up it looks like your linked Codesandbox is still in draft mode and isn't publicly visible

@Dosant
Copy link
Author

Dosant commented Sep 20, 2024

As a heads up it looks like your linked Codesandbox is still in draft mode and isn't publicly visible

Thanks, should work now https://codesandbox.io/p/sandbox/minimal-report-react-18-legacy-forked-27c5qj?workspaceId=05eaf057-3cbb-4e04-a1ab-b97be769b1bc

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

No branches or pull requests

2 participants