Skip to content
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

editorView.state is out of sync until rerender of ProseMirror has completed #4

Closed
michael opened this issue Jan 8, 2021 · 16 comments
Closed
Labels

Comments

@michael
Copy link
Contributor

michael commented Jan 8, 2021

Hi there!

I'm integrating ProseMirror with your approach using React. Along with ProseMirror I'm rendering an EditorMenu that computes and renders editor tools.

Here's my integration code:

export default function Editor (props) {
  const [editorView, setEditorView] = useState(null)
  const [state, setState] = useProseMirror({
    doc: DOMParser.fromSchema().parse(buildHTML()),
    plugins: [
      keymap(buildKeymap(schema)),
      keymap(baseKeymap),
      history(),
      dropCursor()
    ]
  })

  useEffect(() => {
    setEditorView(viewRef.current.view)
  }, [])

  const viewRef = useRef()
  return (
    <div className='flex flex-col h-screen'>
      <header className='p-5 bg-white shadow'>
        <EditorMenu editorView={editorView} />
      </header>
      <main className='flex-1 overflow-y-auto p-5 article'>
        <div className='max-w-4xl mx-auto py-6 sm:px-6 lg:px-8'>
          <ProseMirror
            ref={viewRef}
            state={state}
            onChange={newState => {
              // HACK: Update the editor view right away, so in EditorMenu editorView.state reflects newState
              viewRef.current.view.updateState(newState)
              setState(newState)
            }}
          />
        </div>
      </main>
    </div>
  )
}

The menu bar thus is one update cycle behind. I solved this problem by running editorView.updateState(newState) in the onChange handler explicitly. However, I wonder what an idiomatic way of doing this would be? I could pass both state and editorView to EditorMenu, but that complicates the interface a bit. I kinda like that ProseMirror and EditorMenu are part of the same render flow with useProseMirror. In my previous integration the EditorMenu was decoupled and was listening to state updates. I also wonder if there's a better way to provide editorView to EditorMenu. Would be nice not avoid using another component state + useEffect for that. I'm quite new to Function Components — not been using React in a while. 😅 Maybe you have some ideas...

Thank you!

@dminkovsky
Copy link
Collaborator

dminkovsky commented Jan 8, 2021

Hi Michael, thanks for opening this issue!

However, I wonder what an idiomatic way of doing this would be? I could pass both state and editorView to EditorMenu, but that complicates the interface a bit.

Passing editor state down to your EditorMenu and using that would be the idiomatic approach in my opinion, and I think the fact that you are not doing this, but reading the view in your EditorMenu is the reason why you are seeing this delay: the view is not updated with the new state until the next render cycle. That explains why your hack works and why the toolbar is behind one cycle.

I kinda like that ProseMirror and EditorMenu are part of the same render flow with useProseMirror.

Yes, that is what inspired the approach in this library: integrating ProseMirror into the React render flow, having the PM state high up so it can be shared with other components—for example, a menu—in other parts of the render tree. So I am wondering: why does passing the editor state complicate your interface?

Here is what I would do:

export default function Editor (props) {
  const [state, setState] = useProseMirror({
    doc: DOMParser.fromSchema().parse(buildHTML()),
    plugins: [
      keymap(buildKeymap(schema)),
      keymap(baseKeymap),
      history(),
      dropCursor()
    ]
  })

  const viewRef = useRef()
  return (
    <div className='flex flex-col h-screen'>
      <header className='p-5 bg-white shadow'>
        <EditorMenu 
          editorView={viewRef.current.view} 
          isBold={isBold(state)} 
          isItalic={isItalic(state)}
        />
      </header>
      <main className='flex-1 overflow-y-auto p-5 article'>
        <div className='max-w-4xl mx-auto py-6 sm:px-6 lg:px-8'>
          <ProseMirror
            ref={viewRef}
            state={state}
            onChange={setState}
          />
        </div>
      </main>
    </div>
  )
}

function isBold(state) {
    return isMarkActive(state, schema.marks.strong);
}

function isItalic(state) {
    return isMarkActive(state, schema.marks.em);
}

// https://github.com/ProseMirror/prosemirror-example-setup/blob/afbc42a68803a57af3f29dd93c3c522c30ea3ed6/src/menu.js#L57-L61
function isMarkActive(state, mark): boolean {
    const {from, $from, to, empty} = state.selection;
    return empty
        ? !!mark.isInSet(state.storedMarks || $from.marks())
        : state.doc.rangeHasMark(from, to, mark);
}

In my example above, I compute the isBold and isItalic properties and pass them to the menu, but you can just pass the entire state to the menu and compute these properties inside that component. This is idiomatic React and running the menu state from editor state this way seems clean and simple to me.

@michael
Copy link
Contributor Author

michael commented Jan 8, 2021

Thank you so much for your answer. I guess I will go with passing both state and editorView. I already have a prosemirrorUtils module with helpers like those. So I'll use those from within EditorMenu but with the correct state.

editorView={viewRef.current.view} unfortunately does not work, since viewRef.current is undefined on the first render. Guarding it is also not enough, since I need editorView to be available after the initial render. That's why I had the useEffect in place. I could pass the viewRef to EditorMenu and then access viewRef.current.view there. However, found that a bit ugly for an interface. Passing a function prop instead (getEditorView) also feels unnatural to me. What do you think?

@dminkovsky
Copy link
Collaborator

Hi Michael, you're welcome. Glad that made sense.

Yes, sorry I was rushing and the editorView={viewRef.current.view} was a typo. I meant to type, as you suggest, passing the viewRef to EditorMenu and accessing it there. This is a matter of style, but personally I don't think that it is ugly. Though I can see how you would: it is a reference and references feel internal, maybe? Does it feel like you're violating the Law of Demeter somehow? I don't think that's the case when (i) you're passing the ref top-down, and (ii) are using the ref in callbacks/event handlers so you don't have the problem of it not being available until after the render. Anyway, it is a style thing and if you think it's better encapsulating it in a function that's good too, why not?

@michael
Copy link
Contributor Author

michael commented Jan 14, 2021

Hey just wanted to say again thank you! I've sticked with setState in the end, as it just feels more natural. But I can say I've managed to come up with a cursor dependent overlay, as well as modal dialogs etc. in a very minimal way using your approach!

I'll do an iteration or two more and then would love to share some code, and have your opinion. And maybe you want to use it as a more complete integration example. I favor your little module much over more complete and opinionated React-Frameworks for ProseMirror. 👏 Think people should know! :)

@michael
Copy link
Contributor Author

michael commented Jan 17, 2021

@dminkovsky i'm currently struggling with implementing autosave using Function Components. I want to debounce the save, and use some logic to make sure only one request is made at a time. This was my previous stateful class-based implementation:

class MyEditor extends React.Component {
  constructor (...args) {
    super(...args)
    this._debouncedAutosave = debounce(this._save, 1000)
    this._saving = false // while saving is in progress
    this._scheduleSave = false
  }

  onContentChanged() {
    this._debouncedAutosave()
  }

  async _save() {
    if (this._saving) {
      this._scheduled = true
      return // no new saves until current one has finished
    }

    this._saving = true
    await saveArticle(this._getEditorContent())
    this._saving = false
    
    if (_scheduled) {
      this._scheduled = false
      this._save()
    }
  }

  render() {
    ...
  }
}

But this gets very messy if I try to model it with useState, useCallback etc. Any idea how I could approach this? It would be handy of course to have saving as a boolean state variable and pass it to the menu to render a loading spinner.

Many thanks! :)

@dminkovsky
Copy link
Collaborator

dminkovsky commented Jan 17, 2021

Hi @michael,

Thanks for the update. I was just about to reply to your previous message. That is great to hear. Yes, a more involved example would be great for the README, or maybe in something like /examples, or perhaps best of all, a CodeSandbox. I would also like to have an example with, for example, setting marks on a selection using a toolbar button.

Regarding the debounced save: I my implementation, I have a debounced save, too. However, I also have a lot of other things going on (inline attachments, link previews, auto delete when an editor is made blank, and more), I found that a state machine using XState was the only way I could manage the all the complexity. XState makes the debounced save problem pretty easy because it handles all the event handling, delayed state transitions, and cleanup. This comes at the expense of learning XState, which has a learning curve.

So, assuming you just want to use React, here's how I would do it. It's the same with a function component as a class component. With class components you store refs on the class instance and and cleanup with componentWillUnmount. With a function component you store refs with useRef and cleanup with a function returned from useEffect(). Using a functional component:

import 'prosemirror-view/style/prosemirror.css';

import React, {useRef, useEffect} from 'react';
import {schema} from 'prosemirror-schema-basic';
import {useProseMirror, ProseMirror} from 'use-prosemirror';

import persist from './persist'

const TIMEOUT = 1000;

return function MyEditor() {
    const [state, setState] = useProseMirror({schema});
    const persisted = useRef(state.doc);
    const timeout = useRef();
    const isPersisting = useRef();
    // Cleanup
    useEffect(() => () => clearTimeout(timeout.current), [])
    return <ProseMirror state={state} onChange={handleChange} />;

    function handleChange(state) {
        // Always update state
        setState(state);
        // Don't schedule a persist if we're persisting or if the document was not updated (selection could have been updated)
        if (isPersisting.current || persisted.current.doc.eq(state.doc)) {
            return;
        }
        // Cancel a pending persist if it exists
        clearTimeout(timeout.current);
        // Schedule a pending persist
        timeout.current = setTimeout(() => {
            isPersisting.current = true;
            persist(state).then(() => {
                isPersisting.current = false;
                persisted.current = state.doc;
                if (!persisted.current.doc.eq(state.doc)) {
                    // TODO: persist again if the doc has been changed while persist was taking place
                }
           ).catch(() => {
                isPersisting.current = false;
            });
        }, TIMEOUT);
    }
};

Taking this one step further without introducing something like XState, I like using Observables instead of Promises because they can be cancelled. So if persist returns an Observable, you can make sure that state updates that happen as a result of the persist can be properly cancelled/cleaned up. Let me know if you're interested in an example of that.

@michael
Copy link
Contributor Author

michael commented Jan 18, 2021

Oh thank you very much! Again, refs to the rescue. :)

I like using Observables instead of Promises because they can be cancelled. So if persist returns an Observable, you can make sure that state updates that happen as a result of the persist can be properly cancelled/cleaned up. Let me know if you're interested in an example of that.

I'd be very curious how this works yes. So you mean, that would simplify the code, as in you always cancel an active request (if necessary) and starting a new one, that finishes only if the user is idle long enough?

However, I also have a lot of other things going on (inline attachments, link previews, auto delete when an editor is made blank, and more), I found that a state machine using XState was the only way I could manage the all the complexity.

Sounds familiar. ;) I've spent the last years working on a scientific editor, with auto-numbering of references and all that stuff. We built our own library for that, Substance.js. However with my new project I'm starting with a rather simple domain model for the editor.

What kind of editing app are you working on?

@michael
Copy link
Contributor Author

michael commented Jan 18, 2021

This is what I came up with now. Let me know if you spot errors or have ideas what could be improved.

return function MyEditor() {
  ...
  // we need an additional instance variable here, so we can access the latest state within the handler
  const latestState = useRef(state)

  function schedulePersist (scheduledState) {
    // Don't schedule a persist if we're persisting or if the document was not updated (selection change)
    if (isPersisting.current || persisted.current.eq(scheduledState.doc)) {
      return
    }

    clearTimeout(timeout.current)

    timeout.current = setTimeout(async () => {
      isPersisting.current = true
      try {
        console.log('saving...')
        await persist(scheduledState)
        console.log('saving done')
        persisted.current = scheduledState.doc
        isPersisting.current = false
        timeout.current = null

        // Compare scheduledState (at time original persist started) vs latestState.current (=now)
        if (!scheduledState.doc.eq(latestState.current.doc)) {
          console.log('new changes meanwhile, rescheduling...')
          schedulePersist(latestState.current)
        }
      } catch (err) {
        console.error(err)
        isPersisting.current = false
      }
    }, TIMEOUT)
  }

  function handleChange (newState) {
    // Close all active popovers
    setActivePopover(null)
    latestState.current = newState
    schedulePersist(newState)
    setState(newState) // Triggers the UI update
  }
}

It's a bit hard to wrap your head around what variable holds which state at what time. E.g. as it turned out, I can only access refs (=instance variables) reliably within the handler functions. For everything else I need to consider the original time of invocation from the Component function. I tried to make this more obvious by using names newState, scheduledState and latestState. Still, if the logic would become more complex than this, maybe class components are better suited for this kind of thing.

Anyway, what I learned in this is:

  • useState() is for state that is UI relevant, set triggers a rerender
  • useRef for state-machine-ish things, no rerenders triggered.
  • If I want to pass "instance variables" to views (e.g. isPersisting to EditorMenu), I need to consider that they will only update on the next state update. In our case this onChange + setState is called on every keystroke so this should be fine. Still, hard to wrap your head around it.

@michael
Copy link
Contributor Author

michael commented Jan 18, 2021

One question: Let's say I have two state variables (useState) and my handeChange needs to modify both on invocation. Then, I would always deal with 2 rerenders right? Possibly I want to optimise each handler function to change only one state variable while it is running and put everything else into refs. Alternatively, use a state management solution that allows for optimising that (not sure if that makes things easier to read though). What's your preferred solution for such cases?

@dminkovsky
Copy link
Collaborator

dminkovsky commented Jan 18, 2021

I'd be very curious how this works yes. So you mean, that would simplify the code, as in you always cancel an active request (if necessary) and starting a new one, that finishes only if the user is idle long enough?

If you use Promises, if you've scheduled fn to run when the Promise resolves (.then(fn)), there is no first-class way to cancel a function running if something changes and you don't want fn to run. You can have a flag that is checked inside fn, but in my opinion that is messy and pollutes code as the code grows complex.

For example, say you want to avoid updating state after a component is unmounted (you will see memory leak warnings if you update state after a component is unmounted), you could use a didUnmount flag and check its state in the handler.

function Component() {
  const didUnmount = useRef();
  const [state, setState] = useState(true);
  useEffect(() => () => (didUnmount.current = true), [])
  return <button onClick={async () => {
    await persist();
    if (!didUnmount.current) setState(t => !t)
  }} />
}

Or you could return an Observable from persist():

function Component() {
  const subscription = useRef();
  const [state, setState] = useState(true);
  useEffect(() => () => {subscription.current?.unsubscribe()}, [])
  return <button onClick={() => {
    subscription.current = persist().subscribe({
      next: () => setState(t => !t)
    })
  }} />
}

The difference between the approaches may seem stylistic, and perhaps it is, but something about cleaning up a scheduled function with a reified, explicit API (unsubscribe()) feels a lot cleaner than checking a flag. It may very well be style though, and the important thing is that you use one of these methods to make sure things you don't want to happen do not happen.

This is what I came up with now. Let me know if you spot errors or have ideas what could be improved.

This is the kind of code that's hard to run in your head because of all the time dependencies, but superficially it looks good to me. As I was alluding above, you may find that you will need to cancel anything that happens after await persist(scheduledState)` if something happens while the persist is occurring. But yes, looks good to me...

I can only access refs (=instance variables) reliably within the handler functions

Yes, I see why you need to have a latestState ref in order to actually access the latest state due to JS closures. I missed that in my example (sorry, I just typed it out without testing 😀).

Still, if the logic would become more complex than this, maybe class components are better suited for this kind of thing.

In my opinion, class components and function components offer the same programmability and opportunities for organizing your code, so which you use is up to taste. Classes let you store things on and access this, so you can have refs and callbacks on this and therefore the callbacks that don't suffer from variables that are closed over because they access them on this. But I like the hooks API a lot, which is only available in function components. I also don't like using this. But it's a matter of taste—I think they're the same.

Anyway, what I learned in this is:

Yep, all of that sounds right.

One question: Let's say I have two state variables (useState) and my handeChange needs to modify both on invocation. Then, I would always deal with 2 rerenders right?

During normal React callback handlers, you will not have a re-render for each call to setState. Multiple calls to setState are batched, updates occur asynchronously and changes to state are rendered out once. I believe I've read that many times in the docs.

However, there is something unusual about the way ProseMirror interacts with React such that if you call setState from a ProseMirror callback, state is updated synchronously. That is, (i) you call setState on one line; (ii) If you access that state on the next line, it has already been updated! I'm not sure if this means this change has already been rendered out to the DOM. But it is unusual and I would like to find out more about this phenomenon.

What kind of editing app are you working on?

I'm working on messaging app. It's a non-instant messenger that sends and receives messages once a day. The idea is it gives you space and time to have meaningful, thoughtful correspondence in a world where all communication media has become "chat".

@dminkovsky
Copy link
Collaborator

By the way, I meant to ask but once again was in a hurry and forgot: what is your editor for? As I mentioned, I am working on a messenger and I put this up on GitHub because this is core to my users' experience.

@michael
Copy link
Contributor Author

michael commented Jan 19, 2021

I'm working on messaging app. It's a non-instant messenger that sends and receives messages once a day. The idea is it gives you space and time to have meaningful, thoughtful correspondence in a world where all communication media has become "chat".

Love the idea. Certainly an interesting challenge to find the sweet spot between a temporal discussion (chat-like) and a larger piece of text to share (like a document). Would love to use such a thing. :)

I am building a web platform for personal exchange of experiences. Like you can write a piece that tells your personal story about a certain topic and also includes concrete action steps for readers to follow. So it is all about learning by experience, rather than learning by facts. You can set a price for your piece and that way ensure readers that give you a bit more of a commitment. Then you can also share more personal stuff, as parts of the article are private. I have a bunch of interesting people that are writing up their stories so i have a few good pieces for the launch of the platform.

It is in German but you can check out the current prototype here:

https://www.letsken.com

Currently I'm doing a proper rewrite in React/Next.js. Would be happy to show you more, and exchange some ideas. :) I'm also having a messaging system there, so you can respond to stories privately and that way get in a conversation with the author.

@dminkovsky
Copy link
Collaborator

I am building a web platform for personal exchange of experiences.

That is very cool. What sort of experiences are you envisioning to be shared? Are the people who share going to be leader types? Like the kind of people who would do a TED talk or anyone? Very nice to have people pay for them: so much content on the Internet is throw-away stuff.

It is in German but you can check out the current prototype here

It looks very nice! Don't know if the name has special meaning in German (guessing it does?) but it sounds good in English.

Currently I'm doing a proper rewrite in React/Next.js.

That's my platform, too. I am one person, so I am also building mobile apps with Capacitor out of the same codebase. Very difficult to make a good mobile experience with Capacitor, but after a lot of time spent I think it is acceptable. I hope, at least :).

Would be happy to show you more, and exchange some ideas.

Definitely.

@michael
Copy link
Contributor Author

michael commented Jan 20, 2021

That is very cool. What sort of experiences are you envisioning to be shared? Are the people who share going to be leader types? Like the kind of people who would do a TED talk or anyone? Very nice to have people pay for them: so much content on the Internet is throw-away stuff.

Definitely one or another TED-talker would help to get the ball rolling. :) However, I'd like to cover topics by people who are not well represented yet on the web. E.g. women doing untypical work (like dealing with hot metal or something :D). I just keep my eyes open for opportunities to encourage people sharing their stories. I'm just assisting, I want to make it their marketplace.

Some good, well researched tech-related stories would also be nice btw. Like how to be a successful solo-entrepreneur in tech. And cover things like how to make technical decisions so you still be able to steer the ship alone if needed. Maybe we can work on something together? :D

It looks very nice! Don't know if the name has special meaning in German (guessing it does?) but it sounds good in English.

Thank you! Ken is an old english term, and i use it as "knowledge by experience":

image

That's my platform, too. I am one person, so I am also building mobile apps with Capacitor out of the same codebase. Very difficult to make a good mobile experience with Capacitor, but after a lot of time spent I think it is acceptable. I hope, at least :).

Really cool you have such a broad expertise! Do you think you'd have some resources left in the summer to help me (like when Ken.app is due). :D

I'm particularly good at UI/UX and product design. So if you need some feedback on your messaging app, I'd be happy to help. You already saved me many hours with your answers in this thread! Thanks a lot again! :)

@dminkovsky
Copy link
Collaborator

That's very cool, I'd never seen "ken" used like that, or as anything but the name, actually.

Yeah, I don't know if I'd call it a broad range of expertise, but more like the opposite? My lack of iOS/Android knowledge drove me in the direction of Capacitor. I've learned a minimum about iOS development, and could really use some more knowledge or a developer who's well versed in mobile development to assist. At the same time, though, I think it's pretty cool to work from one codebase to develop all the apps. If I gain any momentum, it will be quite easy to also add an Electron app, for example. I'm also not sure how, even with a large team, I would have handled rich text editing across native apps or pseudo native apps like with React Native. With this one codebase, it's just one editor—ProseMirror—and therefore one format on all platforms. I kind of like it.

Do you think you'd have some resources left in the summer to help me (like when Ken.app is due).

We'll see. Hopefully I'll be 100% with my project, but who knows.

I'd be happy to help. You already saved me many hours with your answers in this thread! Thanks a lot again! :)

Thanks a lot. Really appreciate it. Will definitely let you know when there is something to test. Are you on iOS? Android?

@stale
Copy link

stale bot commented Apr 17, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 17, 2021
@stale stale bot closed this as completed Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants