-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Delayed setState(value) results in Cannot resolve a DOM point from Slate point #3575
Comments
Thanks for posting this issue. I think this is caused by the selection state of the editor briefly being in a state that isn't possible to achieve in its value state because they are being applied out of sync. So, understanding that the onChange here is contrived and not going to usually include setTimeout (?), my recommendation is to ensure that your onChange loop is as tight as possible and if you must really do something that takes a meaningful amount of time, subscribe to onChange only to get updates and don't pass that value back to slate so it can continue to control its own internal value One "fix" for this bug is to move the selection state back into the value state. However, that's a non trivial change and wont be done any time soon, if at all. It was a deliberate decision to keep the selection state internal. But a lot of people have these issues so maybe over time it'll be enough to justify that transition |
Hi guys. I'm pretty new with Slate (a few weeks), but the current behavior looks reasonable for me. I'd be grateful if you can explain what's wrong with it. So, how I see it:
Would be great to hear which of the points above aren't correct and why. |
Same issue (Chrome Latest). Typing too fast (intentionally) will cause the problem above. In my case, I'm updating the cache inside useEffect hook, React blames the component though. Wonder if there's a fix here. |
I feel like Slate should wait to move the cursor until the new value has been received, I think Quill does the same |
I would argue that this is by design. Why would you want your UI to have an intentional lag for your user when they type? If you need to do anything async with your editor value you should separate it from the user's UI state. In my Slate implementations I do something like this: const [value, setValue] = useState(/* ... */)
const onChange = newValue => {
setValue(newValue);
if (newValue !== value) {
// set newValue in redux state, post to server etc.
}
} |
As pointed out by @phamstack, this state mismatch can happen even on seemingly synchronous operations. React state updates are by nature asynchronous, so there's isn't any guarantee that they will run quickly enough to satisfy Slate's requirements. The possible solutions I can think of are:
|
While you are correct that React updates are by nature asynchronous, Slate uses the If you could point out in a code-sandbox a "seemingly asynchronous" operation that causes this issue we could see if there is anything to fix here. |
I haven't a reproduction example for that case, maybe @phamstack can provide one? |
This is a problem when doing collaborative editing. Say user A has focus on line 2. User B removes the line above (line 1). As far as I can tell there is no way I can adjust the selection for user A before So far I have forked slate-react and just fail silently, and then adjust the selection myself. |
An option to silently fail, and maybe fallback to the closest valid position would probably a great idea |
I have the same problem described by @skogsmaskin. My application already has collaboration built-in, which powers other widgets (slate is just one of them). I am opting out of slate's collaborative handling and instead piggybacking on my own app's infrastructure to make things more uniform This error bites me constantly - when collaborating and also when using undo/redo (which, again, works through my own app code for consistency rather than using slate's system). User types paragraph A, then types paragraph B, then clicks I also cannot store/reset the selection as part of the undo/redo mechanism, unless I use slate's system. |
…tion this is a problem Track issue: ianstormtaylor#3575
In a collaboration session this would happen all the time. Find some way of dealing properly with this. Track issue: ianstormtaylor#3575
@gabriel-peracio , could you provide a little bit more information about how you're handling these external transformations? If you're going about this by mutating the value manually via immer and injecting it back in with |
Slate core has a couple of ways to deal with this:
So perhaps the React editor should expose the ability to include selection in the state, as the core does, but I also wonder if we need some documentation around getting started with collaboration. The Slate design goal is (as far as I can tell) to synchronise based on operations, not state, for the best user experience. |
In a collaboration session this would happen all the time. Find some way of dealing properly with this. Track issue: ianstormtaylor#3575
In a collaboration session this would happen all the time. Find some way of dealing properly with this. Track issue: ianstormtaylor#3575
Same issue here with remote updates as @skogsmaskin & @gabriel-peracio described, as current workaround I'm removing selection during the remote update (collaboration mode or history updates), and set it back once value updated: React.useEffect(() => {
setValue((currentValue) => {
/**
* Slate can't properly handle selection range for async updates.
* https://github.com/ianstormtaylor/slate/issues/3575
*
* We need to reset selection only for real remote updates
* (collaboration or undo/redo events)
* In case of direct update currentValue will be update before remote
* so they will be equal.
*/
if (currentValue !== remoteValue) {
editor.selection = null;
}
return remoteValue;
});
}, [editor, remoteValue]);
React.useEffect(() => {
if (elementSelectedForEdit && !editor.selection) {
const lastCharPosition = Editor.end(editor, []);
Transforms.select(editor, lastCharPosition);
ReactEditor.focus(editor);
}
}, [editor, elementSelectedForEdit, value]); In this example it's expected that focus will be moved to the end of the string even if update happens somewhere in between (I think that this ux is better in the case if you have batching logic for remote updates), but in case if you want to keep last selection position, just store it (i.e. the ref object) and check in second hook that this selection valid for current range and set after. But in general I think this issue should be fixed in the slate itself. |
It sounded like @czechdave was requesting a reproduction case that we would expect to behave more or less synchronously. I wonder if this would serve: https://codesandbox.io/s/condescending-surf-rzcin. It's essentially the boilerplate from the React docs, except that it stores editor state in a Recoil atom. While Slate works as expected with editor state managed by React's Recoil is new and a little funky, but its updates normally happen in the same lifecycle as |
I'm doing something like notion when the user types '/' then a popup appears to select a block type and after selecting a block I want to remove that slash so I'm taking a backup when user types '/' and then resetting the value with backup after selecting a block, So how can I achieve this without passing a value to slate? |
@rohankeskar19 You could use the Transforms interface to remove the current line. |
Is there a fix for this yet, I really need this please |
@rohankeskar19 there isnt a bug to fix here really, its that you need not try to intercept or delay the onChange event. |
I think trying to abstract away the selection turned out to be a mistake, and it is now too expensive to fix. I'm not sure this is a "bug" per se. On the one hand, it's fine to tell people to not modify the onChange event - on the other hand:
In any case this was an architecture decision that robbed a ton of flexibility from the API I'm not a collaborator on this project nor particularly familiar with the codebase, so my opinion should be taken with a grain of salt. For what it's worth, I ended up deciding on prosemirror as my editor instead of slate - I felt like I was fighting with slate every time I tried to do something even slightly non-obvious |
But in collaboration when I pass new value from database, It causes issues with the selection, Is there a workaround for that? |
Theres a lot of questions I have with how youre trying to use the onChange event, but here are some general rules:
Edit: I missed the collaborative editing question. This issue is not related to that. For collaborative editing, you will probably need to reconcile the selection but im not sure specifically how you should do that. I recommend you open a github question and bring it to the #collaboration channel on slack. |
I don't have any issues with sending the data inside onChange, What I'm saying is when I change the value passed to Editable in the value prop, I'm also getting the same error
Here I'm passing state.blocks to the value prop which will be coming from database |
@rohankeskar19 sorry, I missed your question. Its kind of tangential to this issue though. |
As far as I can tell, the Might also need to rework the slate provider code in https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/src/components/slate.tsx a bit so it actually directly handles a re-render instead of doing this through the parent component (changing the |
It's still needed to read the editor contents, but yes Modifications to |
@TheSpyder The Slate provider does rely on the
key dependency) which is where the misunderstanding comes from I think. Most react developers are used to components being either controlled or uncontrolled, this sits kind of somewhere inbetween. Although it is not clear to me why it is done this way.
(I made a PR to demonstrate what I mean #4540) |
I'm not a React developer, so I can't really comment on why the Slate component was done that way. I am only familiar with the Slate core, how it works and what it expects :) |
Do you want to request a feature or report a bug?
What's the current behavior?
Repro: https://codesandbox.io/s/slate-reproductions-1sbiz
Slate: 0.57.1
Browser: Chrome
OS: Mac
What's the expected behavior?
The above repro shows how a delayed
setState(value)
call systematically results in the errorCannot resolve a DOM point from Slate point
.In my case, I'm using Apollo to keep some text in sync with the server, I have optimistic updates in place, but it's still not quick enough to not trigger this error.
I also encountered the same issue while trying to sync with some quite large (and badly written) Redux store.
The text was updated successfully, but these errors were encountered: