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

useRemoteCursorOverlayPositions in react 18 does not work #372

Open
ilya2204 opened this issue Nov 11, 2022 · 11 comments
Open

useRemoteCursorOverlayPositions in react 18 does not work #372

ilya2204 opened this issue Nov 11, 2022 · 11 comments
Assignees
Labels
bug Something isn't working investigation-needed

Comments

@ilya2204
Copy link
Contributor

Hello! useRemoteCursorOverlayPositions doesn't seem to work in react 18. In the simplest setup with the withCursors plugin, when a remote user types, an error occurs. Maybe the plugin does not support react 18, but I did not find any information about this. (When I switched to version 17 everything worked as it should)
image

@ilya2204
Copy link
Contributor Author

In @slate-yjs/react@0.2.4 it seems react 18 worked

@BitPhinix BitPhinix self-assigned this Nov 13, 2022
@BitPhinix BitPhinix added the bug Something isn't working label Nov 13, 2022
@superBertBerg
Copy link

That only occurs for me if I'm editing at the end of a line my guess: RemoteCursorOverlay is updating before the document is in sync

@BitPhinix
Copy link
Owner

Could you share a reproducible example of it not working? Just tested bumping the example and it seems to be working there without any issues

@BitPhinix BitPhinix added investigation-needed and removed bug Something isn't working labels Nov 18, 2022
@ilya2204
Copy link
Contributor Author

ilya2204 commented Nov 20, 2022

@BitPhinix Here not working example in my fork https://github.com/ilya2204/slate-yjs/tree/not-working-example

slate-yjs.-.Google.Chrome.-.20.November.2022.mp4

@BitPhinix
Copy link
Owner

Ahaha, I forgot to switch over to ReactDOM.createRoot, my bad 😅

@BitPhinix BitPhinix added the bug Something isn't working label Nov 21, 2022
@FindAPattern
Copy link

It seems like this issue may have to do with React 18's automatic batching. I've been looking into this for a couple of days, and noticed that the editor state seems to be behind the DOM state. getDOMPoint looks at the rendered DOM text nodes to compute valid positions. It appears that the automatic batching causes the DOM nodes to get out-of-sync with the editor state.

I've been able to work around this (hopefully temporarily) by disabling automatic batching during changes. This is really hacky, but it seems to solve the issue:

const { onChange } = editor;
editor.onChange = () => {
  flushSync(() => {
    onChange();
  });
};

Hopefully this helps someone!

@rashadaziz
Copy link

It seems like this issue may have to do with React 18's automatic batching. I've been looking into this for a couple of days, and noticed that the editor state seems to be behind the DOM state. getDOMPoint looks at the rendered DOM text nodes to compute valid positions. It appears that the automatic batching causes the DOM nodes to get out-of-sync with the editor state.

I've been able to work around this (hopefully temporarily) by disabling automatic batching during changes. This is really hacky, but it seems to solve the issue:

const { onChange } = editor;
editor.onChange = () => {
  flushSync(() => {
    onChange();
  });
};

Hopefully this helps someone!

Hi, thank you for this solution. However, I noticed that this causes a warning about flushSync getting called inside a lifecycle function, is this okay or will it cause problems going forward?

@FindAPattern
Copy link

It seems like this issue may have to do with React 18's automatic batching. I've been looking into this for a couple of days, and noticed that the editor state seems to be behind the DOM state. getDOMPoint looks at the rendered DOM text nodes to compute valid positions. It appears that the automatic batching causes the DOM nodes to get out-of-sync with the editor state.
I've been able to work around this (hopefully temporarily) by disabling automatic batching during changes. This is really hacky, but it seems to solve the issue:

const { onChange } = editor;
editor.onChange = () => {
  flushSync(() => {
    onChange();
  });
};

Hopefully this helps someone!

Hi, thank you for this solution. However, I noticed that this causes a warning about flushSync getting called inside a lifecycle function, is this okay or will it cause problems going forward?

I haven't seen any problems on my end, but it's definitely not an ideal solution (the internal components should properly handle batching). I'm using it as a stopgap.

@prabir-vora
Copy link

prabir-vora commented Mar 13, 2023

Facing this same issue. Unfortunately in my case, I am mounting the Editor only after my HocusPocus Provider has synced so even before flushSync is used with onChange, my editor crashes. This is a work around to issue with Initial State conflict - #111 and #385

Crash Scenario:

User A already has an active selection.
User B initializes Editor and as soon as editor mounts, I get the error message from getOverlayPosition.

I am temporarily running React in v17 by using ReactDOM.render as a workaround as suggested in #374

@BitPhinix Have you been able to probe deeper into this?

@gregfunteratwoconnect
Copy link

gregfunteratwoconnect commented Mar 15, 2023

same issue as well on my end using Plate to mount when the HocusPocus Provider synced.
Tried as well the hack to add flushSync but it seems its still crashing on my end.
Anyone has an idea how for to fix this rather than downgrading to react v17

     <PlateProvider<MyValue>
        editor={editor}
        initialValue={content}
        onChange={(newValue) => {
          flushSync(() => {
            setContent(newValue)
          })
        }}
      >

image

@gregfunteratwoconnect
Copy link

Might help someone else but it seems I am able to fix this on my end without downgrading to v17 by using useLayoutEffect

  useLayoutEffect(() => {
    setShowCursors(true)
    return () => {
      if (!reportEditorRootRef?.current) {
        setShowCursors(false)
      }
    }
  }, [])
 <div
      ref={reportEditorRootRef}
    >
      <PlateProvider<MyValue>
        editor={editor}
        initialValue={content}
        onChange={handleOnChangeContent}
      >
          <div
            ref={reportPlateEditorRef}
            styles={{ backgroundColor: 'white', position: 'relative' }}
          >
            <Plate<MyValue>
              editableProps={{
                style: {
                  padding: '.4in',
                  width: '100%',
                  height: '100%',
                },
              }}
            >
              {showCursors && (
                <ReportRemoteCursors containerRef={reportPlateEditorRef} />
              )}
            </Plate>
          </div>
        </div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigation-needed
Projects
None yet
Development

No branches or pull requests

7 participants