useEditorEffect runs before node views render

[tanishqkancharla]

Hey! I noticed that useEditorEffect runs before node views have been rendered. This is causing a bug where the editor effect tries to do something that is only valid after the node view finishes rendering...have you all noticed/thought about this or have a recommended fix?

I think I can cobble together a fix where I defer the effect by having a node view let a context know when it finishes rendering. And then I can run the effect. Seems a little hacky + similar to soemthing you guys have already with the useLayoutGroupEffect, although I'm not sure that's related. Any suggestions?

[smoores-dev]

Hmmmm.. Right. I think (it's been a minute) that the order of events is:

• The ProseMirror component is rendered
• The EditorView is updated (in a useLayoutEffect). This is when the NodeView React elements are created/updated, but they are not synchronously rendered.
• useEditorEffect effects run
• NodeView React elements are rendered

Does that match what you're experiencing?

Can you share a bit more about what you're trying to do in your useEditorEffect that's causing issues?

[tanishqkancharla]

Yep, that matches my experience. Consider a widget that needs to reposition itself via view.coordsAtPos but the coords are only available after node view renders because the position is inside them...(so they default to null or 0,0, i think)

[tilgovi]

In what component are you trying to use useEditorEffect? Using it within a node view component should be safe, but using it elsewhere and assuming some node view is rendered may not be. It may or may not be possible for us to make that guarantee, but I would have to think a bit more about it.

[smoores-dev]

Right; if you need to obtain the coords of some position, you likely need to use useEditorEffect without any dependencies, so that it runs on every render. We do this in a few places in NYT, and I also have some other projects that have this need. This should be sufficient, I'm pretty sure; the effect should re-run after each render of the node view components. It does mean that there will be a brief moment before the first render where you might get an out-of-place position, but it should automatically correct itself on the next render.

[tanishqkancharla]

So a few notes: I don't have dependencies declared on my effect. And the incorrect behavior i.e. the position not being computed correctly only happens when the nodeview is created or destroyed i.e. via input rule. Once it's in there, position gets computed correctly.

I was building a repro of this, but I ran into a potentially more pressing bug. Here's the demo: https://stackblitz.com/edit/stackblitz-starters-ffsnxj?file=src%2FApp.tsx

Repro:

1. Type "a" into the editor
2. Press backspace

The editor throws an error "The object cannot be found here". Looking at the console, looks like it some garbage collection issue since it's happening in native code.

[tilgovi]

I think I've seen something similar even in the demo of this repo! I'll look into it and see if we can get that addressed as its own issue. Thanks!

[tanishqkancharla]

In what component are you trying to use useEditorEffect? Using it within a node view component should be safe, but using it elsewhere and assuming some node view is rendered may not be. It may or may not be possible for us to make that guarantee, but I would have to think a bit more about it.

Ah, I missed this message previously. This is my problem. I'm doing the positioning in a child element of the editor but assuming the nodeviews are rendered when I perform that positioning.

Is there a simple workaround here? Otherwise, I might either try rendering my widget as a part of the schema (ew) or having a context that node views call into after rendering (and then runs the positioning effect)

[tilgovi]

Is there a simple workaround here? Otherwise, I might either try rendering my widget as a part of the schema (ew) or having a context that node views call into after rendering (and then runs the positioning effect)

The approach we've generally used is to just run the positioning code on every render, and if it can't find its target it can do nothing.

I might be able to help more if I can make a reproduction. Ignoring the issue of deleting the first and only paragraph, can you build out the stack to show me what you're doing more?

[tilgovi]

Just layout out some background for all of us here, to help understand what might be happening:

The layout effect that updates the ProseMirror editor view is in the useEditorView hook, and our components deliberately wrap the <LayoutGroup> higher in the component tree than that (this is the reason for <ProseMirrorInner>). The reason to do that is so that <LayoutGroup> effects flush after the editor view is up-to-date.

However, I find React's scheduler is a little bit hard to reason about sometimes. We can rely on it to flush the <LayoutGroup> effects (including ones registered with useEditorEffect) after any ProseMirror update, but maybe not only after. I think all we maybe have guaranteed is that the group/editor effects will run after an update, but we haven't guaranteed that they won't run before as well.

[tilgovi]

A little more information: to force group effects to run, we force an update in the layout group whenever an effect in the group needs to be created or destroyed. We try to do that conditionally, to limit re-renders. So, we don't force multiple updates in a single cycle. Unfortunately, during a top-down update of everything, React doesn't give us any good lifecycle hook for knowing that we're about to update anyway. If we could skip the forced update altogether, that might solve our problem, but I was too worried that any way we tried to do that would be too hacky and brittle.

[tanishqkancharla]

Sorry for the late message. I amended the stackblitz to do a repro, but oddly, I don't see the behavior I reported here. I'm going to dig into why later.

Here's my demo. Basically, the red dot is supposed to be tracking the cursor.

The behavior I'm seeing in my editor (the widget stops tracking as soon as a new node view is created e.g. as soon as I press enter) is not there but there's still a bug: The widget stops tracking if you do cmd-delete then start typing. In fact, it seems like the effect doesn't run at all (see logs). This only happens when the shortcut goes to the beginning of the node view: alt-backspace works fine if you're not going to the beginning of the paragraph, but if you are, it breaks in the same way as cmd-delete.

[tanishqkancharla]

It's also kind of in a weird spot when the doc is empty and the cursor is all the way at the beginning, but I can live with that.

[smoores-dev]

@tanishqkancharla Sorry that this has languished for so long without response, but your issues here and in #42 actually were the driving force behind the new architecture that we've been working on! If you have a moment, I would be interested to hear whether you're still seeing issues with this approach on the new beta version, 0.3.0-beta.0. There are some changes to the API; the updated README is here.

[tanishqkancharla]

No need to apologize @smoores-dev! This is open-source, after all :) I really appreciate the work you guys have put into this.

Unfortunately, I think I'm still running into the problems: I modified the above demo to use the new beta version. As soon as i start typing, I get an error (position -1 out of range). If you look at the logs, it seems like useEditorEffect is still running before the paragraph renders. In fact, the paragraph component doesn't render at all until I start typing. Am I doing something wrong?

[smoores-dev]

Ah! Ok, I finally took a look at this. I highly recommend changing your schema from doc: { content: 'paragraph*' } to doc: { content: 'paragraph+' }; you'll notice that all of the ProseMirror documentation does this as well. With it, ProseMirror will automatically fill in your initial doc node with a single empty paragraph; without it, it won't try to insert the paragraph until after you've tried to add some text. You almost always want the first situation! I will take a deeper look at this index error, because I still don't think it should actually be erroring, but I am pretty confident that that schema change will do what you want!

[tanishqkancharla]

Ah yes you're completely right. That was a mistake on my part. Thanks!