Slate: shouldNodeComponentUpdate doesnt catch document.data update

[klis87]

Do you want to request a _feature_ or report a _bug_?

Potential bug

What's the current behavior?

I have some global settings in slate object which I keep in document.data. Some nodes depend on those settings and should rerender on change.

I implemented the following function to make it rerender:

shouldNodeComponentUpdate: (previousEditorProps, editorProps) => {
    if (
      previousEditorProps.editor.value.document.data.get('variables') !==
      editorProps.editor.value.document.data.get('variables')
    ) {
      console.log('variable changed, lets rerender');
      return true;
    }

    return undefined;
  },

The problem is that this function will never be called with old and new variables, on change previousEditorProps already contains new variables. Is it intended behaviour or a bug? If this is a bug, I will make some interactive example.

What's the expected behavior?

previousEditorProps should be called once with old and new variables which would allow me to rerender all custom nodes.

[alanchrt]

I'm seeing the same behavior in my app, attempting to compare data on a node before and after changes.

My guess is that maybe all these props aren't deep copies of all the nested objects, just keeping references at the top level? From some rough comparisons of object references, it seems that's the case.

If they're just references, I can't think of a way shouldNodeComponentUpdate would be useful for any use case. @ianstormtaylor, are you aware of any apps successfully using shouldNodeComponentUpdate, and what are their use cases? Are we wrong to expect to be able to get snapshots of previous and current editor Value?

My own version (which always fails to return true):

    shouldNodeComponentUpdate(previousEditorProps, editorProps) {
      const previousNode = previousEditorProps.node;
      const { editor, node } = editorProps;
      const { nodeHeadlines } = editor.props;

      if (!nodeHeadlines) {
        return;
      }

      const previousDocument = previousEditorProps.editor.props.value.document;
      const { document } = editor.props.value;

      const previousHeadlineKey = nodeHeadlines[previousNode.key];
      const headlineKey = nodeHeadlines[node.key];

      const previousHeadline = previousHeadlineKey &&
                               previousDocument.getNode(previousHeadlineKey)
      const headline = headlineKey && document.getNode(headlineKey)

      const previousLevel = previousHeadline && previousHeadline.data.get('level');
      const level = headline && headline.data.get('level');

      if (previousLevel !== level) {
        return true;
      }
    },

[alanchrt]

I wonder actually if deep copying editor in renderEditor before passing it into the Content as props (which later make it down to Node props and shouldNodeComponentUpdate) will make it possible to do a comparison. I wonder if it's just always the same object being mutated, whereas the other props are immutable copies.

Not sure. I'll do some playing around with it when I get a moment. The good news is that renderEditor is a plugin hook, so if copying there works, it wouldn't have to make it into slate upstream necessarily to get it functional. That said, maybe it makes sense upstream too.

[alanchrt]

I could see how a deep copy wouldn't help, however, if editor was being mutated before renderEditor anyway, because then the old object would already be mutated, and we'd just be copying the updated version. Maybe not the solution.

[klis87]

If this is helpful, this is a handler in which I change data:

editor.change(change =>
    change.setNodeByKey(editor.value.document.key, {
      data: {
        ...editor.value.document.data.toJS(),
        variables: values.variables,
      },
    }),

[ianstormtaylor]

Hey folks, this sounds like something that should use React Context instead?

[klis87]

@ianstormtaylor we could, but then what is the purpose of shouldNodeComponentUpdate, maybe we should remove it then?

[skogsmaskin]

I didn't have much success with shouldNodeComponentUpdate lately. I currently use a hack with invisible decorators to update the nodes. I have some props that come from React that should update the nodes (but aren't and shouldn't be part of the actual node data). So I just build some decorators for the relevant nodes and set them on the value, and the nodes updates themselves. I find this pattern much more reliable and easier to work with with than how shouldNodeComponentUpdate currently works. You don't have to write throughout tests, and the nodes will only update when actually needed. I love the fact that slate-react is very restrictive on updating the nodes in the first place performance wise.

[alanchrt]

Yeah, I actually ended up just hopping over to redux (like @skogsmaskin's decorators) to always subscribe to changes I care about, which isn't great because of the coupling in my plugin components. It makes them less portable between editors. I may fall back to context since it slipped my mind that context even existed as an option.

But, then, yeah, what's the purpose of shouldNodeComponentUpdate? Maybe it does make sense to remove it completely unless there's a clear, practical use case? Seems like it's been a misleading sinkhole for several of us, when maybe we would have gravitated toward alternative data flows from the start if it wasn't even available.

[ianstormtaylor]

I'm down to remove it I think. If people can verify that all of this can be handled with React's built-in context (or maybe Redux, dunno don't use it). Or some other standard userland pattern. I agree that shouldNodeComponentUpdate is a hack.

[klis87]

@ianstormtaylor I used React context and it solved all issues, so personally I don't see any use for shouldNodeComponentUpdate if we can use just React to achieve rerendering of a node any level deep in any moment, unless this method was created for some other cases too?

[alanchrt]

I just ported everything to context today, too, and all seems well. Kill it? Nobody jumped at it in chat to mention any use case they have for it, and there are no example use cases in the repo.

[ianstormtaylor]

Can someone post code of what they were using shouldNodeComponentUpdate for before? and the new way they are handling it with context?

[alanchrt]

@ianstormtaylor I didn't hang onto my shouldNodeComponentUpdate implementation for anything since it never really solved my use case, but here's an example (among several) of where I'm now using context to update components when relevant data changes (without something that normally triggers a re-render).

Here, BlockContext passes its consumers a beginKey, which is set to the key of the beginning delimiter's text node for the currently-focused "greater block" (a blockquote, code block, etc.)

My BeginKeyword component switches to the non-Empty font (text becomes visible) whenever the related greater block is focused, then switches back to Empty if focus moves to another area outside the block.

You can see it in action by focusing the QUOTE or SRC block here (hard refresh if you visited before).

import React from 'react';
import { css, cx } from 'react-emotion';

import { BlockContext } from '../../../context';

const style = css`
  font-family: 'Empty';
  line-height: 16px;
`;

const focusedStyle = css`
  font-family: 'Roboto Mono';
`;

const BeginKeyword = ({ isFocused, attributes, children }) => (
  <span {...attributes} className={cx(style, { [focusedStyle]: isFocused })}>
    {children}
  </span>
);

const ContextWrapper = (props) => (
  <BlockContext.Consumer>
    {({ beginKey }) => {
       const { node } = props;
       const isFocused = (node.key === beginKey);
       return <BeginKeyword {...{ isFocused, ...props }} />
     }}
  </BlockContext.Consumer>
);

export default ContextWrapper;

Before, I would have tried using editor.props to pass beginKey into shouldNodeComponentUpdate, then compare to the previous value and see if it had changed. I would re-render all relevant nodes when it changed. In the case of the one above, it would determine if beginKey matched during the render. This never really worked as hoped since the previous editor.props weren't stored.

[thesunny]

FYI, I was successfully using shouldNodeComponentUpdate before.

After upgrading, it stopped working. previousProps and props always return the same value now but it didn't before.

[ianstormtaylor]

I believe that this may be fixed by https://github.com/ianstormtaylor/slate/pull/3093, which has changed a lot of the logic in Slate and slate-react especially. I'm going to close this out, but as always, feel free to open a new issue if it persists for you. Thanks for understanding.