fix: default to rendering the editor immediately, while staying backward compatible #5161
Conversation
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| } from 'react' | ||
|
|
||
| import { Editor } from './Editor.js' | ||
|
|
||
| const isDev = process.env.NODE_ENV !== 'production' | ||
| const isSSR = typeof window === 'undefined' | ||
| const isNext = isSSR || Boolean(typeof window !== 'undefined' && (window as any).next) |
There was a problem hiding this comment.
This was the closest thing I could find to detecting that you are running Next.js (SSR or CSR)
nperez0111
force-pushed
the
fix/react-rerenders
branch
2 times, most recently
from
2ae6496
to
8b9819b
Compare
The use case of a selector would be two-fold. For preventing re-renders by allowing you to only re-render when that selected state changes (e.g. not on every transaction like it is by default), and to give a more convenient API for tiptap state. I haven't updated the PR yet, but I envision an additional API like: function MyComponent() {
const { editor, state } = useEditorWithState({
selector: (currentEditor) => {
return { isBold: !currentEditor.can().setBold() };
},
equalityFn: deepEqual
});
return (
<>
<Toolbar>
<button>{state.isBold ? "Bold" : "Not Bold"}</button>
</Toolbar>
<Editor editor={editor} />
</>
);
}With the code above, the component only has to re-render when the editor switches from being able to bold the current selection and not being able to. Which drastically will reduce re-renders. |
|
Understood, that is super! I think it would make sense to have a selector API on the function MyBubbleMenu() {
const isBold = useCurrentEditor({
selector: editor => !editor.can().setBold(),
});
}
<EditorProvider>
<MyBubbleMenu />
</EditorProvider>Or, with a "useEditorState" hook function MyBubbleMenu({editor}: {editor: Editor}) {
const isBold = useEditorState(editor, {
selector: editor => !editor.can().setBold(),
});
}
function App() {
const editor = useEditor();
return (
<>
<MyBubbleMenu editor={editor} />
<EditorContent editor={editor} />
</>
);
} |
Yep, will be adding something like that to it. Just haven't gotten around to updating this PR yet |
nperez0111
force-pushed
the
fix/react-rerenders
branch
4 times, most recently
from
a26ed74
to
b65ac57
Compare
nperez0111
force-pushed
the
fix/react-rerenders
branch
from
b65ac57
to
607e939
Compare
🦋 Changeset detectedLatest commit: 9c7363a The changes in this PR will be included in the next version bump. This PR includes changesets to release 54 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…nstance simultaneously
nperez0111
force-pushed
the
fix/react-rerenders
branch
from
204e7c5
to
f157449
Compare
|
This example proves the difference between the default behavior & the new behavior enabled by the new |
|
🎉Amazing work, thank you!!!🎉 |
|
Weirdly it seems that the overloads for It seems to be fixed if the overload with
Otherwise the result from |
|
Hm, I ran into this myself. I think you might need |
| export function useEditorState<TSelectorResult>( | ||
| options: UseEditorStateOptions<TSelectorResult, Editor | null> | ||
| ): TSelectorResult | null; | ||
| export function useEditorState<TSelectorResult>( | ||
| options: UseEditorStateOptions<TSelectorResult, Editor> | ||
| ): TSelectorResult; |
There was a problem hiding this comment.
Do you mean useEditor or useEditorState?
Because if I go into node_modules and just flip the order of these overloads (in the .d.ts file of useEditorState) it seems to be working all fine. I think this is because TypeScript will resolve the Editor overload first, and if it's null it will resolve the Editor | null second. But since Editor DOES extend Editor | null, it resolved the nullable one first. Sorry that's a bit of a mouthful to read😆
I'm wondering if just flipping the order of the overloads in the source code will fix it? Would it be possible to make another changeset & pre-release to test?
Happy to make a PR as well if I haven't explained it very well 😄
There was a problem hiding this comment.
Oh I was talking useEditor, Hm interesting. I would happily take a PR for it!
There was a problem hiding this comment.
Gotcha. will make a PR
Fwiw, typescript IS able to infer the boolean to true exactly, but only because it was explicitly written out in the overloads. Here's an example of that behaviour
|
Thank you for the tremendous work! I wonder if anyone can provide insight into possible circumstances where |
@Nantris Hm, It causes nothing to render? That is strange, I mean I thought I tried all of the cases. If doing SSR it must be false, if doing CSR it can be true (for better typing). Does the editor instance not get returned back to you? Are you on an older React version maybe? |
|
I’m using Next.js. Which version will this be rolling out in, and will there docs for this change? |
|
Currently released in the latest beta version (which you can try this out with) Definitely will be writing docs for it, just isn't on the main release yet so haven't gotten around to it. We've been working on our release structure (moving to changesets and CI pipelines) so we just haven't been confident to do a stable release just yet. Would very interested here how it is working for you though. For Next.js would amount to |
|
Chiming in as I'm also using Next.js, I've split the entire editor into a separate component, and then imported it dynamically, like this: // components/editor.tsx
import {useEditor} from '@tiptap/react';
export function Editor() {
const editor = useEditor({immediatelyRender: true, shouldRerenderOnTransaction: false});
// ...
}// pages/editor.tsx
import dynamic from 'next/dynamic';
const Editor = dynamic(
() => import('../components/editor.tsx').then(mod => mod.Editor),
{ssr: false},
);
<Editor />I've had this setup from before the changes in this PR, however there were no issues once I upgraded. Worth noting that this, of course, changes when/where the code for the Editor is bundled and this may not be your desired behaviour. Though one advantage is that the editor only mounts in the browser anyway, and now you don't ever have to deal with null checks as the editor will always exist by the time you're rendering. |
|
Thanks for your reply @nperez0111! After having some time to look into this, I think the issue must be internal to TipTap somehow. It's quite curious. I see you tested in Vite and that's where I do my development. There's no SSR involed. The When I enter <div contenteditable="true" translate="no" class="tiptap ProseMirror" tabindex="0">…</div>But curiously, when I right-click it and "Reveal in Elements Panel" it tells me, React is on I wonder if you have any theory what the issue could be? Thank you again for the stellar work! |
|
Hm, nothing off the top of my head should have been different, could you double check with the previous beta (maybe something changed in another beta?) So maybe check @tiptap/react@2.5.0-pre.13? Other than that, I've heard from others that it is working properly so I am unsure why it'd be different, could you share your code? If not maybe a reproducible example? Maybe just delete features until you have it working? |
Hey! First, a big thank you for taking the time to prepare this PR, @nperez0111! We observed similar behavior to that described by @Nantris while working in Strict Mode. Essentially, the editor displays an empty |
Great hint, I put it in StrictMode and it is not working for me either. I will look into what is happening |
|
I can confirm our issue is Strict Mode related as well. Great find! |
…ounted This forces the editor to re-use the editor instance that existed prior to an unmount and remount of the same component. This fixes a bug in `React.StrictMode` introduced with the latest performance updates by PR #5161
|
Great, I think I have a fix with: #5338 |
Changes Overview
A third PR for performance of the useEditor hook 😅.
What I've done here is combine a few of the different approaches and added a bit of my own spin to it:
Prior art:
What I've done here is to take a spin on #4579 and base it on a
useSyncExternalStoreinstead (for efficient re-rendering handled by react).While adding the performance optimization made by #4453 (re-using existing editor instances, and just updating their options if they change).
It is backward-compatible to people who use SSR (printing a warning and telling them they should set the new flag explicitly for forward-compatible but only in dev mode).
It is forward-compatible, where if you don't specify the new flag, people will automatically have the editor on first render.
If the flag is specified, you get better TS types since it is now guaranteed that you get an editor instance.
If you include the
selectfunction, you can effectively choose when the component should re-render by returning something different than selected before (inspired byuseSelectorin react-redux)If you include the
equalityFnfunction, you can choose how to compare yourselectfunction's return value (again inspired byuseSelectorin react-redux)Implementation Approach
I've introduced 3 new options to the
useEditorhook (and subsequently the editor provider).immediatelyRenderwhich is to control the new vs. the old behaviorselectwhich is to give the consumer fine-grained control over rendering by selecting the values they care about in re-rendering the editorequalityFnwhich defines how to compare the values that the consumer generates with theselectfunctionWhen not specifying the
immediateRendervalue, if it is detected to be in SSR mode (i.e. no window object) it will be assumed to befalseto not break Next.js users (but printing a message that they should be explicit about this), otherwise it will be assumed to betruereducing flicker in existing applications.Eventually, when we have confidence to do so (i.e. the next major version) we can throw an error instead and have Next.js users (and other ssr frameworks) have to explicitly opt out for their use case (or perhaps a new hook).
Testing Done
I tested in the demos using all possible options and in a next application here is a table for reference:
Testing this sort of thing in react can be a little complicated (since you need to control when react re-renders) but if someone knows of a way to test this I'm happy to add tests for it.
Verification Steps
Run the demos and set different values for
immediatelyRenderand in a Next.js app.Additional Notes
I'm unsure what version tiptap claims to be compatible with, and I know older versions do not have
useSyncExternalStorebut I also know thatreact-reduxuses a shim to make it compatible with older versions.Checklist
feat: Implement new featureorchore(deps): Update dependencies)Related Issues
#5001