BitPhinix / slate-yjs

Yjs binding for Slate
https://docs.slate-yjs.dev
MIT License
514 stars 73 forks source link

useRemoteCursorOverlayPositions in react 18 does not work #372

Open ilya2204 opened 2 years ago

ilya2204 commented 2 years ago

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 commented 2 years ago

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

superBertBerg commented 2 years ago

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 commented 2 years ago

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

ilya2204 commented 2 years ago

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

https://user-images.githubusercontent.com/52297646/202913795-c9c5603e-e369-4025-a881-452a7d67ca9b.mp4

BitPhinix commented 2 years ago

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

FindAPattern commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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>