-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix React Dependency Array creating new editor instances instead of reusing existing one #4453
Conversation
This commit adds a check to not create a new editor instance whenever the deps change but update the editorOptions object via editor.setOptions
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@bdbch Friendly bump! Noticed that this PR has gone stale. We’re running into this exact issue in our app, where if React rerenders, it causes the editor to unmount and then remount. Hoping to see this merged soon. |
@svenadlung could you take a look? |
packages/react/src/useEditor.ts
Outdated
|
||
return () => { | ||
isMounted = false | ||
} | ||
}, deps) | ||
}, [deps]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line wrongly double-wraps the dependencies in an additional array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - fixed
I noticed an issue that I commented on. In theory this seems like exactly what we need to avoid the insanity of passing a zustand store to our Extensions just to give them up-to-date functions to read from, or otherwise passing down all of their relevant arguments. In practice it doesn't seem to work that way though. But even after implementing this with the line I commented on fixed, it seems like the option passed to an Extension are not updated. Eg we are trying to get data from an external store but the function is still using the outdated version of the store. I'd expect that Specifically I'd expect Also, just for reference, this will break any code that relied on the editor re-rendering on every update of the editor, eg for a I hope this will get merged into |
This is close to the approach I’m taking right now for the Mention suggestions list to be reactive, except I used Legend State. I’m also unsure if this PR being merged will mean I no longer have to do that. I’m not familiar with TipTap’s internals and how all their extensions are authored. But in my specific case, I was interested in using the Mention extension and wanted the suggestions list to be reactive. Reading their code for the Mention and Suggestion extensions, it seems that, potentially, the issue is caused by suggestions being passed into Prosemirror as a prop. I’m not schooled enough in Prosemirror internals to know if this is exactly the issue, but I gathered from a Prosemirror forum thread that, unlike React, Prosemirror views don’t update when props change, only when state changes. So, if you pass in something that you expect to change, you lose reactivity once you pass it down. If this is indeed the case, then it means TipTap needs to do more work to ensure props being passed to Prosemirror work more like props in React, so that Prosemirror internals (which is supposed to be abstracted away from us) don’t leak out into our app. The suggested fix was to pass an observable object down. This fix worked in my case, using Legend State to create an observable for the list of suggestions and then passing it over to TipTap’s Mention extension. Using Legend State, I was able to preserve reactivity as data is passed from React to Prosemirror and back to React again (since the Suggestion extension renders a React component), which is nice. The issue of TipTap not playing nice with regular React state/props, however, remains. What I expect is for the editor not to destroy itself and be recreated just for props to update. I expect TipTap to hook properly into Prosemirror so that props passed into it are reactive in a way one would expect any React component to be. |
@bdbch @svenadlung would love to see this get merged 🙌 |
Right now tiptap/packages/core/src/Editor.ts Lines 159 to 164 in 6f6e49e
Before this PR the editor instance actually gets completely replaced which would mean that the extension manager will also run through another time. Could you maybe createa Codesandbox / Stackblitz showcasing what you want to do with Prosemirror plugins accessing external data, I could try to use that to work off from.
I think that would be a case yes - there's no easy way to persist a plugin state.
That's true - @svenadlung what your thought on that? That would mean we need to push the version up to 3.0.0 for the next release with this PR merged.
This PR won't fix that as the mention plugin won't rerun (see above). But when it comes to updating Prosemirror props & state the tiptap/packages/core/src/Editor.ts Lines 159 to 164 in 6f6e49e
In general I think there are a few misconceptions right now when it comes to reactivity and the Feel free to send in Sandboxes which we can use to work from as references or maybe even open a PR for that issue if you find a good fix for this - I'd be down to play around with those reactivity problems. Back to the suggestion issue: What I did was completely doing data fetching / reactivity inside my React SuggestionList component rendered inside my suggestion popup which was reacting to query changes. That means I didn't use the items array at all. (Not the cleanest solution but worked). |
Resolved with: #5161 |
Please describe your changes
Before this PR everytime dependencies change the whole editor gets replaced with a new instance. This would remove entered content, cause flashes and gets rid of the editor state (which sucks for example because of the history living in the state for example)
This PR implements a check that will update the editor options when the deps change instead of creating a new instance with said options.
How did you accomplish your changes
Added a check in the react useEditor hook to update the editor instead of replacing it.
How have you tested your changes
I tested my changes on the demo page I created (Link: https://deploy-preview-4453--tiptap-embed.netlify.app/src/issues/2403/react/)
How can we verify your changes
See above
0
my-editor
my-editor updated-editor
Remarks
On my tests everything worked fine for now but I think we'd need to verify that this change is not breaking some special cases.
Checklist
Related issues