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

Out-of-order updates with React Concurrent Mode #3334

Closed
jaredpalmer opened this issue Dec 17, 2019 · 15 comments
Closed

Out-of-order updates with React Concurrent Mode #3334

jaredpalmer opened this issue Dec 17, 2019 · 15 comments

Comments

@jaredpalmer
Copy link
Contributor

jaredpalmer commented Dec 17, 2019

Bug

What's the current behavior?

When using Slate with Concurrent React (via ReactDOM.createRoot) on the experimental release channel), updates (to the node list?) can occur out of order resulting in text being appended after the cursor.

Kapture 2019-12-17 at 11 39 22

https://codesandbox.io/s/slate-050-basic-x-concurrent-mode-bug-qnxqu

Slate: 0.55.3
React: Experimental, 16.8.6 with unstable_createRoot
Browser: Chrome
OS: Mac

What's the expected behavior?

Updates stay in order no matter how fast the user types.

Suggested Solutions

It seems like this could have to do with either batchedUpdates or scheduler, but I'm not exactly sure. I don't know enough about the internals of Slate to give more detail, but worth getting ahead of this IMHO.

@jaredpalmer jaredpalmer changed the title Out-of-order updates with concurrent mode Out-of-order updates with React Concurrent Mode Dec 17, 2019
@jaredpalmer
Copy link
Contributor Author

Looking through source, it looks like Slate is using batchedUpdates within editor.onChange. I'm wondering now, if this is because react state is too slow for this high frequency update, and the value needs to be kept on a ref?

@jaredpalmer
Copy link
Contributor Author

jaredpalmer commented Dec 17, 2019

Update: It seems like this can be somewhat mitigated by using unstable_discreteUpdates within the change handler.

export const SlateInput = () => {
  const [value, setValue] = React.useState('');
  const handleChange = val => {
+    ReactDOM.unstable_discreteUpdates(() => {
      setValue(val);
+    });
  };

  const editor = React.useMemo(() => withReact(createEditor()), []);
  return (
    <Slate editor={editor} value={value} onChange={handleChange}>
      <Editable placeholder="Enter some plain text..." />
    </Slate>
  );
};

@jaredpalmer
Copy link
Contributor Author

jaredpalmer commented Dec 17, 2019

For those interested, I solve this by only storing/serializing the debounced editor value by about 200ms.

Update: this doesn't work either.

@ianstormtaylor
Copy link
Owner

@jaredpalmer do you have any learnings that can be added to core to mitigate this? I have no idea what unstable_discreteUpdates does.

@jaredpalmer
Copy link
Contributor Author

I don’t know what it does either, but it seems to work better, but not perfectly if you use it.

@dminkovsky
Copy link

dminkovsky commented Dec 18, 2019

Looks similar to #3236. Have you experienced the autocorrect issues shown there?

I solve this by only storing/serializing the debounced editor value by about 200ms

What do you mean by that? If you debounce setValue(), doesn't the slate editor not update after keydown?

Have you tried newer experimental builds?

@jaredpalmer
Copy link
Contributor Author

@dminkovsky are you talking about react? or slate? yes, I've tried with multiple experimental builds of react. no luck. I still have not figured out how to get it to work. I really really like the new 50+ api, but looking at other options at the moment.

@dminkovsky
Copy link

@jaredpalmer I was talking about React, just because you mentioned 16.8.6 and I thought experimental builds were based on more recent versions. This past week I had to back off of concurrent mode entirely and went back to to 0.47 as well, despite also liking the 0.50 API a lot more, too.

@jaredpalmer
Copy link
Contributor Author

cc @gearaon not sure if you’re the right person, but putting this on your radar

@amorriscode
Copy link

I was using Slate (0.57.1) successfully with react@0.0.0-experimental-f6b8d31a7 but I updated Next.js (due to a security issue) recently and now I'm seeing this issue as well.

@kyarik
Copy link

kyarik commented Apr 6, 2020

@jaredpalmer Thanks for mentioning ReactDOM.unstable_discreteUpdates. That led me to do some research. discreteUpdates definitely helps, but as you pointed out, it's not perfect, as there are still some cases in which the cursor gets left behind.

ReactDOM.unstable_discreteUpdates runs the function you provide with a UserBlockingPriority, which is a high priority, but not the highest.

On the other hand, ReactDOM.flushSync and ReactDOM.flushControlled run the function you provide with ImmediatePriority, which is the highest priority. ReactDOM.flushControlled is still unstable, so it's available as unstable_flushControlled.

Therefore, a possible fix to the issue is to wrap setValue(val) in either of those two functions. So,

const handleChange = useCallback(val => {
  ReactDOM.flushSync(() => {
    setValue(val);
  });
}, []);

or

const handleChange = useCallback(val => {
  ReactDOM.unstable_flushControlled(() => {
    setValue(val);
  });
}, []);

With this change, the original issue seems no longer reproducible.

BTW, you misspelled @gaearon, so I don't think he got notified about this thread.

@gaearon
Copy link

gaearon commented Apr 6, 2020

Yeah I didn't see it. :-)

@circlingthesun
Copy link
Contributor

This seems to be working fine with the latest slate and experimental version of react https://codesandbox.io/s/slate-0604-concurrent-mode-3x30j

@gaearon
Copy link

gaearon commented Apr 7, 2021

Yeah, we fixed this

@gaearon
Copy link

gaearon commented Apr 7, 2021

Basically, we now run updates synchronously by default, and even if we're going to relax this in some future majors for some events, we'll keep that behavior for keyboard, clicks, and so on. And then if you want to opt out, useTransition is your entry point to time slicing.

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

No branches or pull requests

7 participants