Implement usePathRef and useNodePath hook #5930
Conversation
|
|
Interesting hook! I'm not sure whether using path refs for this is a good idea, though. I would only recommend it if it gives significantly better performance than simpler solutions based on export const useNodePath = (node: Node): Path => {
const selector = useCallback(
(editor: Editor) => ReactEditor.findPath(editor, node),
[node]
)
return useSlateSelector(
selector,
(a, b) => a ? Path.equals(a, b) : false,
{
// Defer the selector until after `Editable` has rendered so that the path
// will be accurate.
deferred: true,
}
)
}Note that in some circumstances, the solution above might render once with the old, incorrect path before it immediately re-renders with the correct path. This is due to the timing of |
|
@12joan Thank you for the proposal. Interesting. Well i thought why not re-using code that is anyways dealing with changes of paths. So my proposal will only get triggered, when a path has actually been changed. Your proposal actually needs to "refetch" the path on every editor change and check equality of prev and the actual path. Which will also include selection only changes or text only changes. But it's very much possible i am thinking a bit too far, and performance is just good enough here. |
There was a problem hiding this comment.
Edit: Sorry, was busy reviewing and didn't see your reply. I've replied below.
I've left some comments. Overall, I quite like the idea of being able to attach a callback to a path ref; it would be great if we had this on point refs and range refs too. It's just using this in a React hook that I'm less sure of.
| const editor = useSlateStatic() | ||
| const [, setCacheKey] = useState(0) | ||
|
|
||
| const pathRef = useMemo(() => { |
There was a problem hiding this comment.
useMemo isn't guaranteed to run only once. In some cases, it will discard the old value even when the component hasn't unmounted yet, which will lead to a memory leak here.
To achieve the same behaviour, I would consider doing everything inside useEffect, or if you need more control over the timing of when the path ref is created, use one or more useRef hooks and top-level if statements to keep it up to date.
There was a problem hiding this comment.
@delijah Please could you mark as resolved any of these review comments that are finished? GitHub won't let me mark them as resolved, but you should be able to. 😄
| // eslint-disable-next-line no-redeclare | ||
| export const PathRef: PathRefInterface = { | ||
| transform(ref: PathRef, op: Operation): void { | ||
| transform(ref: PathRef, op: Operation): (() => void) | void { |
There was a problem hiding this comment.
Since the caller already has the PathRef object and can access its onChange and current directly, how about we return a boolean here instead of a callback, indicating whether the path was changed by the operation?
| return | ||
| } | ||
|
|
||
| const prevPath = ref.current ? [...ref.current] : ref.current |
There was a problem hiding this comment.
No need to duplicate the array. Although the current property is mutable, the path object itself never changes.
| const prevPath = ref.current ? [...ref.current] : ref.current | |
| const prevPath = ref.current |
| if (path && prevPath ? !Path.equals(path, prevPath) : path !== prevPath) { | ||
| return () => ref.onChange(path) | ||
| } |
There was a problem hiding this comment.
I found this ternary quite hard to read. I wasn't sure if it was (path && prevPath) ? ... or path && (prevPath ? ...) until I checked in an AST explorer.
If you go with my suggestion above to return a boolean instead, how about splitting this into two if statements to be more readable?
| if (path && prevPath ? !Path.equals(path, prevPath) : path !== prevPath) { | |
| return () => ref.onChange(path) | |
| } | |
| if (path && prevPath) { | |
| return !Path.equals(path, prevPath) | |
| } | |
| return path !== prevPath |
Makes sense. The trade off is that when using path refs, these checks need to happen on every operation for every path ref. I'm not sure if this would be faster or slower. You could try out both solutions in https://www.slatejs.org/examples/huge-document and see? |
- Return boolean instead of function - Break down ternary expression - Remove array duplication
Hmmm, true. So even if you don't use the hook, we have to go trough some computations for every operation. Let's see if i'll find some time to test this.... Anyways. For now, i've added the requested changes. Did not implement the hooks for pointRefs and rangeRefs yet, waiting for another feedback round before doing so. |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const path = useMemo(() => ReactEditor.findPath(editor, node), [node]) |
There was a problem hiding this comment.
Including editor in the deps array should be harmless here
There was a problem hiding this comment.
Although this useMemo might run more often than you're expecting due to the inclusion of node. The node object identity will change on every keystroke inside the node, which will cause findPath to return a new path object, which will cause usePathRef to create a new path ref, all on every keystroke.
You could use ReactEditor.findKey instead, which will return a stable key object that should be the same for the lifetime of the node (see with-dom.ts for how this works).
Element components use this key as the React key anyway, so when calling useNodePath directly inside the component of the element it references, the key should never change.
| }, | ||
| }) | ||
| } | ||
| if (prevPath.current !== path) { |
There was a problem hiding this comment.
See my comment above. This path object might change more often than you're expecting. I would suggest using Path.equals here.
| setCacheKey(prev => prev + 1) | ||
| } | ||
|
|
||
| return pathRef.current?.current |
There was a problem hiding this comment.
| return pathRef.current?.current | |
| return pathRef.current?.current ?? path |
There was a problem hiding this comment.
Not sure why to add that one. But we could replace the ? with a !.
There was a problem hiding this comment.
On second thought, keeping it as ? or using ! is probably fine
| const { current, affinity } = ref | ||
|
|
||
| if (current == null) { | ||
| return | ||
| return false | ||
| } | ||
|
|
||
| const prevPath = ref.current ? [...ref.current] : ref.current | ||
| const prevPath = ref.current | ||
| const path = Path.transform(current, op, { affinity }) |
There was a problem hiding this comment.
How about we do const { current: prevPath, affinity } = ref at the top?
Then since we always return false if prevPath is null-ish, TypeScript will know that prevPath can never be null later in the function.
This would simplify the expression for the return value to return !path || !Path.equals(path, prevPath).
There was a problem hiding this comment.
Even better, we could refactor Path.transform so that it guarantees to returns the original path object if the path is unchanged. (Currently it returns a new path object every time, which is a minor performance issue by itself.) Then the check becomes path !== prevPath, which has almost no performance overhead.
Or maybe you could use path !== prevPath && !Path.equals(path, prevPath), just in case Path.transform sometimes returns a new but identical path object unnecessarily.
|
Ok, i've implemented all the changes - i hope 😅. What i've found while testing the huge document in chrome: With CPU 20x slowdown: There is a noticeable change in performance when typing. With CPU 6x slowdown: There is a slightly noticeable change in performance when typing. With CPU 4x slowdown: There is a very slightly noticeable change in performance when typing. Without any CPU throttling: I can still feel some kind of drag.. but barely noticeable. MacBook Pro (16-inch, 2021) Hmmmm.... yeah. Not sure if it worth the performance decline. It's difficult for me to determine how "bad" it actually is (on a scale from 1 to 10)... what do you think? Ah and sorry, i've implemented that ternary expression again 😅 Couldn't come up with a better solution. Maybe you have an idea again? |
| p[op.length - 1] += 1 | ||
| return p |
There was a problem hiding this comment.
It would be best if we skip the const p = [...path] when it isn't needed, and instead do that just before we use it.
| if (path && prevPath ? !Path.equals(path, prevPath) : path !== prevPath) { | ||
| return () => ref.onChange(path) | ||
| } | ||
| return path !== prevPath |
There was a problem hiding this comment.
I would keep the Path.equals check here as well just in case Path.transform returns a new path unnecessarily in some edge cases. path !== prevPath && !Path.equals(path, prevPath).
There was a problem hiding this comment.
The path === null case can be handled by returning true in the if statement above.
Is this performance change when using I would test by using the dropdown menus on that example to set the number of blocks to something large like 100,000 and checking the average keypress duration under "Statistics". |
|
No it's without using Sorry, didn't find time to continue on this. We've decided to continue with |
2 participants
Description
This PR implements a hook called
usePathRef. The purpose of it is, to be able to get notified about changes on a path. It is achieved by adding a change handler to pathRefs. The reason why we came up with this, is actually, to have a hook calleduseNodePath, which will trigger a re-render, whenever a path of a node changes. It's pretty sensitive, since it could lead to performance issues. It should not be used by default, to retrieve the path for a node, but only when the intention is, to really get notified about path updates. A use-case in our stack is basically, to display a pen icon next to the line/paragraph, where the caret, orselection.focus.path, actually lies.Example
Checks
yarn test.yarn lint. (Fix errors withyarn fix.)yarn start.)yarn changeset add.)I did not yet add a changeset and maybe even tests and docs, since i wanted to have your feedback about this feature first.