Remove intermediate render cycle from useEditor #4579

C-Hess · 2023-10-29T07:55:27Z

~~May or may not be a breaking change depending on if any people actually required this behavior. That would surprise me though.~~

Was a breaking change because it broke SSR, though that's probably not something TipTap should support long term anyways. Instead, opted for an alternative hook to be used until a major version change.

Please describe your changes

Possibly fixes #3345 (issue can probably be re-opened)

We've ran issues getting TipTap operating smoothly with virtual scrolling libraries as well because they ideally expect the height of a component be defined after the first render cycle. While we can't guarantee that Prosemirror doesn't introduce their own delays in rendering, I think the least we can do is remove the unnecessary delay in the React TipTap wrapper.

How did you accomplish your changes

The change really is just moving initialization to the first render cycle. As a nice side effect, this also means you also no longer have to check if the editor is null before using it ❤

How have you tested your changes

~~I ran through most of the tests and they seem to be passing. I spot checked a few test failures and they appear unrelated to the changes made.~~

No longer relevant. New approach does not touch existing hook.

How can we verify your changes

Verification might be a little involved. Didn't really want to get react virtual scrolling all setup in an example at this time, plus like I said, there is no guarantee's it would eliminate the problem entirely. At a minimum though, it removes an unnecessary render cycle.

Checklist

The changes are not breaking the editor
Added tests where possible
- It doesn't really seam testable with the current fixures
Followed the guidelines
Fixed linting issues

Related issues

#3345

Possibly fixes ueberdosis#3345

netlify · 2023-10-29T07:55:32Z

✅ Deploy Preview for tiptap-embed ready!

Name	Link
🔨 Latest commit	`fbf13bd`
🔍 Latest deploy log	https://app.netlify.com/sites/tiptap-embed/deploys/65cfce5f29a47f0008d07249
😎 Deploy Preview	https://deploy-preview-4579--tiptap-embed.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Possibly fixes ueberdosis#3345

C-Hess · 2023-12-08T23:04:47Z

@janthurau , Sorry to put pressure, but any chance that we could get this reviewed? It would be awesome for us because it could potentially help resolve issues with virtual scrolling libraries.

janthurau · 2023-12-09T12:39:57Z

hey @C-Hess, absolutely. We haven't had a look here yet as the tests / build steps are still failing:

7:37:54 AM: ../packages/react/src/Context.tsx(36,26): error TS2322: Type "Editor | null" is not assignable to type "Editor".
7:37:54 AM:   Type "null" is not assignable to type "Editor".

Do you think you can fix them?

C-Hess · 2023-12-13T07:31:29Z

Oops 😳, at one point I thought the build was failing for unrelated reasons. I'll resolve the issue, sorry

ondrejsevcik · 2024-01-23T14:39:38Z

@C-Hess thanks for taking initiative to fix this issue 🙏🏼 highly appreciated.

rfgamaral · 2024-01-23T15:31:55Z

@C-Hess This was implemented before but reverted because it doesn't work with SSR. You may want to go through that PR discussion, and perhaps find a solution that doesn't break SSR, and doesn't introduce others (IIRC, nobody was able to come up with a solution without issues).

For reference, we have a custom useEditor hook in @doist/typist - we don't care for SSR for the time being - but it's outdated compared to the one in Tiptap. So, if we could have the useEditor in Tiptap fixed (SSR support, init on first render, don't cause additional issues), that would be awesome.

Good luck!

ondrejsevcik · 2024-01-24T07:32:30Z

It would be good to hear from the owners of the TipTap to understand their perspective on this topic. Maybe @janthurau knows?

In my opinion it would be better to split it up into two separate use cases. SSR for TipTap editor does not make much sense. The WYSIWYG is component meant to be used in browser and it will always struggle in server side environment. Trying to bend it will bring unnecessary complexity. I would rather encourage to use HTML helpers for SSR instead.

… fix/react-immediate-render

…e-render

C-Hess · 2024-01-28T20:54:01Z

Thanks for all of the useful comments. I didn't know it was attempted before, but it definitely helps to know.

I changed the approach to be non-breaking. In a future major version of TipTap, I'm hoping that useEditorForImmediateRender can replace the logic within useEffect with the caveat that TipTap will no longer be usable through server side rendering (which from my understanding, given the comments above, it shouldn't have been usable via server-side rendering anyways)

… fix/react-immediate-render

packages/react/src/useEditor.ts

C-Hess · 2024-02-16T18:32:01Z

Going to rely on CI for testing as I'm not home yet to verify my updates. I updated the React editor context and main demo file to optionally use the newer hook.

I'll test this later tonight

C-Hess · 2024-02-16T21:16:39Z

Should be good to review @bdbch and @svenadlung. Let me know if you want to tweak the approach I went with here. Thanks!

corbinkems · 2024-02-20T16:59:49Z

I'm also running into this issue and it's causing a lot of headaches. @janthurau

ondrejsevcik · 2024-02-23T08:45:55Z

packages/react/src/useEditor.ts

+      requestAnimationFrame(() => {
+        requestAnimationFrame(() => {
+          if (isMounted.current) {
+            forceUpdate({})


Not required in this PR, but it would be nice to add comment here why is this section necessary (to call the forceUpdate wrapped in 2 requestAnimationFrame calls on every editor transaction). This is a bit black magic to me.

Same here. I just copied this over from the original hook, but it looks to me like it's logic to get around timing issues between Prosemirror transactions and React render cycles

Not sure if this will help, but this seems to fix the flickering issue mentioned here: #2040

Unfortunately there is a habit in this repo of closing non-fixed issues.

@szymonchudy is that issue separate from #5166? It's hard to tell if they're related or truly the same.

C-Hess · 2024-03-12T19:45:37Z

@bdbch , @svenadlung, @janthurau sorry to bother you all. Is there any chance this could be looked at? I see two other PRs introduced by you both that both have different ways of doing this on top of this one. Each approach has pros and cons, but it would be awesome for us to be able to resolve this issue. This is a high priority for my organization to help hopefully resolve virtual scrolling issues when using TipTap, so I'd be more than happy to make any suggested changes as soon as possible if requested.

Thank you all for your hard work on this Repo!

hjoelh · 2024-04-30T20:29:51Z

Ran into this issue today, until this or another fix is merged - my current workaround is like this

  {/* 
      due to https://github.com/ueberdosis/tiptap/pull/4579
      editor is null on initial render 😢, to prevent a flicker we render 
      an empty box with the same styles as the edtior 
      */}
      {editor ? (
        <EditorContent editor={editor} />
      ) : (
        <Box className={styles["editor"]} />
      )}

Nantris · 2024-05-19T01:28:10Z

packages/react/src/Context.tsx

@@ -19,18 +19,38 @@ export const useCurrentEditor = () => useContext(EditorContext)

 export type EditorProviderProps = {
  children: ReactNode;
+  /**
+   * This option will create and immediately return a defined editor instance. The editor returned in the context consumer will never be null if
+   * this is enabled. In future major versions, this property will be removed and this behavior will be the defualt.


Typo defualt to default

Nantris · 2024-05-19T01:32:02Z

@C-Hess would this potentially address #5166? I think probably not based on quickly looking over it, but figured I'd ask to be sure.

nperez0111 · 2024-05-31T13:35:51Z

I took a stab at this, taking a slightly different approach for it: #5161

nperez0111 · 2024-07-11T08:08:06Z

Resolved with: #5161

nperez0111 · 2024-07-11T08:08:45Z

@C-Hess Very much appreciate your contribution, was definitely a source of inspiration for my change

Remove intermediate render cycle from useEditor

a592852

Possibly fixes ueberdosis#3345

C-Hess requested review from bdbch and svenadlung as code owners October 29, 2023 07:55

Remove intermediate render cycle from useEditor

1892b70

Possibly fixes ueberdosis#3345

janthurau force-pushed the fix/react-immediate-render branch from a592852 to 1892b70 Compare November 18, 2023 07:29

Merge branch 'develop' into fix/react-immediate-render

431c0cd

C-Hess added 5 commits January 28, 2024 14:40

Change to non-breaking implementation

cedeee9

Merge remote-tracking branch 'origin/fix/react-immediate-render' into…

a385bb3

… fix/react-immediate-render

Merge remote-tracking branch 'tiptap/develop' into fix/react-immediat…

07b6d59

…e-render

Fix reverting remaining files

994d154

Actually revert all changes to useEditor

d0badc4

utilities link

48e2e28

rfgamaral mentioned this pull request Feb 5, 2024

fix(react): use ref instead of state in useEditor to prevent rerenders #4856

Merged

4 tasks

svenadlung and others added 5 commits February 12, 2024 11:52

Merge branch 'develop' of github.com:ueberdosis/tiptap into develop

8010dfb

Merge branch 'develop' into fix/react-immediate-render

f30bf2a

Update provider to add optional prop for useImmediateRender

ab99fac

Add comment and fix export

f55f7f1

Merge remote-tracking branch 'origin/fix/react-immediate-render' into…

6c50f07

… fix/react-immediate-render

C-Hess commented Feb 16, 2024

View reviewed changes

packages/react/src/useEditor.ts Show resolved Hide resolved

Fix lint

6da0423

Fix build issue. Use memo to prevent re-initialization

fbf13bd

C-Hess mentioned this pull request Feb 21, 2024

fix: initially set editor object only on non-ssr environments #4864

Closed

4 tasks

ondrejsevcik reviewed Feb 23, 2024

View reviewed changes

jtmclucas13 mentioned this pull request Mar 21, 2024

[Bug]: useEditor initially returns null as of 2.2.2, breaking first-render usage #5001

Closed

2 tasks

nperez0111 mentioned this pull request May 16, 2024

fix: default to rendering the editor immediately, while staying backward compatible #5161

Merged

5 tasks

Nantris reviewed May 19, 2024

View reviewed changes

nperez0111 force-pushed the develop branch 2 times, most recently from bcaef5c to 7e7ae19 Compare June 12, 2024 04:33

bdbch deleted the branch ueberdosis:develop June 26, 2024 21:57

bdbch closed this Jun 26, 2024

bdbch reopened this Jun 27, 2024

nperez0111 closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove intermediate render cycle from useEditor #4579

Remove intermediate render cycle from useEditor #4579

C-Hess commented Oct 29, 2023 •

edited

Loading

netlify bot commented Oct 29, 2023 •

edited

Loading

C-Hess commented Dec 8, 2023 •

edited

Loading

janthurau commented Dec 9, 2023 •

edited

Loading

C-Hess commented Dec 13, 2023

ondrejsevcik commented Jan 23, 2024

rfgamaral commented Jan 23, 2024

ondrejsevcik commented Jan 24, 2024 •

edited

Loading

C-Hess commented Jan 28, 2024 •

edited

Loading

C-Hess commented Feb 16, 2024

C-Hess commented Feb 16, 2024

corbinkems commented Feb 20, 2024

ondrejsevcik Feb 23, 2024

C-Hess Mar 12, 2024

szymonchudy May 15, 2024 •

edited

Loading

Nantris May 19, 2024

C-Hess commented Mar 12, 2024

hjoelh commented Apr 30, 2024

Nantris May 19, 2024

Nantris commented May 19, 2024

nperez0111 commented May 31, 2024

nperez0111 commented Jul 11, 2024

nperez0111 commented Jul 11, 2024

Remove intermediate render cycle from useEditor #4579

Remove intermediate render cycle from useEditor #4579

Conversation

C-Hess commented Oct 29, 2023 • edited Loading

Please describe your changes

How did you accomplish your changes

How have you tested your changes

How can we verify your changes

Checklist

Related issues

netlify bot commented Oct 29, 2023 • edited Loading

✅ Deploy Preview for tiptap-embed ready!

C-Hess commented Dec 8, 2023 • edited Loading

janthurau commented Dec 9, 2023 • edited Loading

C-Hess commented Dec 13, 2023

ondrejsevcik commented Jan 23, 2024

rfgamaral commented Jan 23, 2024

ondrejsevcik commented Jan 24, 2024 • edited Loading

C-Hess commented Jan 28, 2024 • edited Loading

C-Hess commented Feb 16, 2024

C-Hess commented Feb 16, 2024

corbinkems commented Feb 20, 2024

ondrejsevcik Feb 23, 2024

Choose a reason for hiding this comment

C-Hess Mar 12, 2024

Choose a reason for hiding this comment

szymonchudy May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Nantris May 19, 2024

Choose a reason for hiding this comment

C-Hess commented Mar 12, 2024

hjoelh commented Apr 30, 2024

Nantris May 19, 2024

Choose a reason for hiding this comment

Nantris commented May 19, 2024

nperez0111 commented May 31, 2024

nperez0111 commented Jul 11, 2024

nperez0111 commented Jul 11, 2024

C-Hess commented Oct 29, 2023 •

edited

Loading

netlify bot commented Oct 29, 2023 •

edited

Loading

C-Hess commented Dec 8, 2023 •

edited

Loading

janthurau commented Dec 9, 2023 •

edited

Loading

ondrejsevcik commented Jan 24, 2024 •

edited

Loading

C-Hess commented Jan 28, 2024 •

edited

Loading

szymonchudy May 15, 2024 •

edited

Loading