Runaway recursion when trying to access editor.value in a plugin onChange event

[ericedem]

Do you want to request a feature or report a bug?

Report a bug

What's the current behavior?

Get a depth exception when trying to access editor.value in a plugin onChange event.

Just try to load this fiddle and it will throw errors: https://jsfiddle.net/uvkrn9ge/1/

Chrome latest Mac OSX.

Note: I did narrow this down to a change in version 4.0.2 of memoize-one. I think this error may have always been happening, it was just covered up.

[ianstormtaylor]

@ericedem what do you need to access editor.value for? Instead you might be wanting change.value which is the current value of the change with operations applied?

[ericedem]

We do a comparison of editor.value vs change.value to determine if we should save the editor value to a database. Though maybe there is a better way to determine if there are changes 🤔

[ericedem]

Basically just following this example: https://docs.slatejs.org/walkthroughs/saving-to-a-database

Comparing change.value.document against editor.value.document since in a plugin we don't have access to the state.

[ericedem]

Ok so, the JSFiddle I linked before doesn't actually reproduce the issue we are seeing in our dev environment. I'm having trouble reproducing in a fiddle or examples, but still working on it.

While debugging, I noticed part of our infinite recursion is coming from this bit of code. When accessing editor.value this piece of code runs:

https://github.com/ianstormtaylor/slate/blob/master/packages/slate-react/src/components/editor.js#L392-L393

Which creates a change on the value and forces all of the plugin change events to run again.
@zhujinxuan Do you remember the motivation behind this bit of code? It seems like a strange side effect of trying to get the value to create an empty change and call a bunch of event handlers, when there wasn't actually a change.

[ericedem]

Ok this will be reproducible with this fiddle once a new version of slate gets published with memoize-one@4.0.2. https://jsfiddle.net/uvkrn9ge/21/

The loop is:

1. onChange() in the plugin gets called initially with the value being set.
2. editor.value is dereferenced, causing the getter in this.resolveChange to get called (mentioned in my last comment)
3. The plugins are then fed an empty Change instance into their onChange() handlers.
4. GOTO: 2

The reason why this wasn't a problem before is that in 4.0.2, the behavior of memoize-one changed such that when you recurse in a memoized method, it doesn't remember what the result is until after the function finishes getting called, which will never happen because the recursion never terminates. So it just keeps seeing the same value and passing the same value check because it was never set. I think this is actually the correct behavior in memoize-one as it makes sure that in recursive methods the original arguments are what g memoized.

What ends up making this even worse is that if you specify a schema in a plugin, the same schema check will also fail in before.onChange() so the change will get an operation added. This means that in the plugin onChange() you can't short circuit if change.operations.size === 0. The workaround we had to do was to pin memoize-one@4.0.2.

I created a simple local slate example based on that fiddle and upgraded to memoize-one@4.0.2 and it does indeed break as we see in our environment. I can push that if people want to look at it.

[zhujinxuan]

I see. The editor.value is unset during a stack run. @ianstormtaylor Shall we provides any compat for users to access editor.value during a stack? (when change.value is accessible).

[zhujinxuan]

@ericedem For database thing, use componentDidUpdate or compare container.state.value with change.value. I would suggest use componentDidUpdate:

  componentDidUpdate(prevProps: Props, prevState: State) {
    const { value } = this.state;
    const { document } = prevState.value;
    const { setStorage  } = this.props;


    if (document !== value.document && onChange) {
    if (this.timeID) {
      window.clearTimeout(this.timeID);
    }
      this.timeID = setTimeout(() => {
        const html = this.serializer.serialize(value);

        if (html === this.cachedHTML) {
          return;
        }

        setStorage(html);
        this.cachedHTML = html;
      }, 50);
    }
  }

[ericedem]

Thanks for the suggestion @zhujinxuan we can add the saving to the container if that's the official recommendation, though we are really trying to encapsulate pure slate behavior into plugins, since our containing component has already grown quite large. With all the components and custom plugins we have several thousand lines of code centered around managing the editor.

I guess the main thing I'm still trying to grok, is why there are events or any kind of side effects being called in a getter, or even a memoized method. Looking at the code where this was added it looks like this code before ran on componentWillReceiveProps() which makes sense, since you would want to normalize props.value any time it changes, and then saved it to local react state. Now it is in a memoized getter, which means that changes get run on access instead of only when the actual props change. This seems like a recipe for unexpected side effects.

I realize that componentWillReceiveProps() is deprecated in React 16, but I'm wondering if this code could still just normalize on prop change. It seems like if we need to normalize before a render, the right way to do it is to transform during getDerivedStateFromProps(), though that is supposed to be a pure function, so making updates to the value and firing onChange() wouldn't be appropriate there 🤔.

Sorry if these are naive musings, I'm a bit new to the codebase and missed the original PR when it was being written 😄

[zhujinxuan]

editor.props.value?

It is doable in previous slate because we used state.value. However it is an anti-pattern as https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization

[ericedem]

editor.props.value

Yep

However it is an anti-pattern

Hmmm, I'm not sure this is an anti-pattern, though after rereading that document getDerivedStateFromProps() may not be a good case for this. Though the memoization example you linked is only appropriate if there are no side effects. Memoization relies on using pure functions.

It seems like the editor would fall under the category of a "controlled" component, and props.value is the source of truth. This is a bit off the rails here, but bear with me:

1. The parent component passes in a value prop.
2. We need to try and normalize this new value before anything else can happen (including render).
3. If there are changes as a result of trying to normalize, we need to wait until the parent updates value again. GOTO: 1
4. If there are no pending changes after attempting to normalize, we are ok to render.

Unfortunately I think this is a bit awkward to do in React now, because it needs to render something, and there isn't a great way to intercept prop changes. Maybe if we kept track of a state.lastGoodValue that could be rendered until things are properly normalized.

[zhujinxuan]

@ericedem How about #2165 ?

[ericedem]

@zhujinxuan That doesn't really fix the problem that this shouldn't be happening at all. I'm inferring from your PR that I should be using editor.props.value instead of editor.value. From an API perspective, if that's the case, shouldn't we just disallow API users from accessing editor.value?

[ericedem]

So I tried just using editor.props.value and that seems to work fine. That will allow us to work around the problem for now.

[zhujinxuan]

Hi @ianstormtaylor , @ericedem just reminds me that process.env.FORBID_WARNINGS has no effect after slate-dev-warning refactor. Shall we add FORBID_WARNINGS back for plugin development and core development?

[ianstormtaylor]

@ericedem I agree with you, the side effects on getters is very hard to understand, and leads to weird situations like this. I'm open to ways to fix this. The only constraint we currently have is that (as you said) rendering needs to always be with the "normalized" value based on the schema. Which creates a little conundrum for us.

@zhujinxuan I'd like to omit the FORBID_WARNINGS, unless we really decide it's an issue, and it can be implemented purely in the testing layer without any changes to the source.

[zhujinxuan]

@ianstormtaylor How about the #2165? We allow user to access the editor.value during onChange, but gives an warning.

[ianstormtaylor]

@zhujinxuan I'm not sure, it feels like it's going to add confusion still. I think the best solution would be to revert to the old way of handling things without the stateful getters if possible.

[ericedem]

One thing I have been thinking of is say we have two values props.value and state.lastGoodValue. We always render state.lastGoodValue. When props.value changes, we run normalizers against it every time it changes until there are no more schema errors, and once we get a run with no errors we set state.lastGoodValue to props.value.

This seems reasonable to me, the only trouble is when someone says editor.value what does that mean?

[ianstormtaylor]

@ericedem I don't think that handles the first render case, where we don't have a state.lastGoodValue to render in the first place?

[ericedem]

@ianstormtaylor Yea that's a good point, it would be null on first render. One thing we could do, is if we had a way to check if a value was "valid" without trying to do normalization, in getDerivedStateFromProps() we could set state.lastGoodValue = props.value if it passes validation. Otherwise we can't render it anyway.

[ericedem]

Oh actually, it would probably be better to do this in the constructor() instead of getDerivedStateFromProps() since we really only need it for first render.

[zhujinxuan]

But there is a problem. We normalize the value by stack.run('onChange', change, this). However, this does not exist in the static getDerivedStateFromProps. The idea of React v16.4 is to avoid this in props->state.

[zhujinxuan]

Hi, @ianstormtaylor @ericedem I make a PR for the state.lastGoodValue feature and refactor the current usage of memoize. I think we shall not take the state, because I think it is unwise to pass this to getDerivedStateFromProps.

PR: #2187

[ericedem]

I don't think we need getDerivedStateFromProps to achieve this. Here is some more concrete pseudocode: https://gist.github.com/ericedem/59d7653a55a767ca68e75f53ef7cbc96

Though I think for performance reasons we could potentially make use of getDerivedStateFromProps since lastGoodValue would be a derived state value by definition. Here is some pseudocode for that: https://gist.github.com/ericedem/d1b1255f4c6d6fee77fa588818bafeb2

[zhujinxuan]

@ericedem The problem of setState with componentDidUpdate is the performance. Here is a issue about it #1938

Using setState in componentDidUpdate will double the updates, significantly harm the capacity of handling large document with decorations. (Though, in current slate, we still experience a significant delay (0.3s~0.6s) when editing with large document + decoration)