Only use Slate Provider's value prop as initial state

[bryanph]

Description
As is made clear by the following issue #3575, the value prop that must be passed to the Slate provider is kind of a hoax because in reality this prop has to be exactly what has been passed to onChange. This changes the Slate provider to relax this requirement. The value prop is now only used on the initial render to initialize the value prop. On subsequent changes it is ignored.

This also clears the way to rename value to initialValue as the prop is really intended to be.

I'm curious whether this misses some subtlety in the rendering code that I'm unaware of. Please do educate me if so 😄

Issue
Fixes: #3575

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: 754bed9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate-react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dylans]

This needs a changeset, and it needs review by a few others as I don't have the history of experience to know with certainty if this change is ok (it seems ok).

[bryanph]

Not sure whether to use a minor or patch for this one 🤔. Settled with minor for now.

[Ghost---Shadow]

This is a breaking change. It should be a major bump.

[Ghost---Shadow]

The documentation needs to be updated as well.
https://docs.slatejs.org/walkthroughs/06-saving-to-a-database

[hassenc]

I'm curious whether this misses some subtlety in the rendering code that I'm unaware of. Please do educate me if so

onChange should not be the only way of changing the Component's value. If you rely on a database to render your content, you'll also want to pass the new value using the value prop. In that case you don't want to rely on a local state that is changed with onChange.

An example would be when you need to display someone else changes to the value.

[dylans]

This is a breaking change. It should be a major bump.

Pre 1.0 beta so not a major bump.

I'm curious whether this misses some subtlety in the rendering code that I'm unaware of. Please do educate me if so

onChange should not be the only way of changing the Component's value. If you rely on a database to render your content, you'll also want to pass the new value using the value prop. In that case you don't want to rely on a local state that is changed with onChange.

An example would be when you need to display someone else changes to the value.

How do you reconcile that with the discussion at #3575 (comment) ?

[Ghost---Shadow]

I am ok with either way. But the documentation over here https://docs.slatejs.org/walkthroughs/06-saving-to-a-database should be updated.

[bryanph]

Besides the docs, the examples should probably also be updated to reflect this change which means just passing initialValue directly to the value prop instead of the value state that is currently kept in all examples.

[natqe]

It is not the right way for an api, you shouldn't changed it. better to have the same signature as the native input api

[KarimNahas]

First of all, I want to say thanks to @ianstormtaylor and @bryanph for maintaining and continuing to build a great library.

Regarding these changes, I wonder whether they actually fix the issue mentioned in #3575: slate used to offer an API similar to any generic input, whereby it exposed props such as value and onChange. This enabled users to interact and maintain the value of the input with a useState hook, as below:

const Editor = () => {
  const [value, setValue] = useState('')
  return (
    <input value={value} onChange={(e) => setValue(e.target.value)} />
  )
}

Consequently, these changes entailed that to be able to interact with the value outside of slate the code would need to be changed from this:

const Editor = () => {
  const editor = useMemo(() => withReact(createEditor())), [])
  const [value, setValue] = useState('')
  return (
    <Slate
       editor={editor}
       value={value}
       onChange={setValue}
    >
      ...
    </Slate>
  )
}

to this:

const Editor = () => {
  const editor = useMemo(() => withReact(createEditor())), [])
  const [value, setValue] = useState('')
  editor.children = value
  return (
    <Slate
       editor={editor}
       value={value}
       onChange={setValue}
    >
      ...
    </Slate>
  )
}

Note the need to manually set the value of editor.children, which if I'm not mistaken can cause a Cannot resolve a DOM point from Slate point. error.

On a separate note, I think changes to the API should be added to the changelog as "Breaking Changes" instead of "Minor Changes" specially if a previous behaviour is no longer supported. I personally find it helpful to use standardised versioning system like Semantic Versioning and Keep a Changelog.

Thanks!

[dylans]

On a separate note, I think changes to the API should be added to the changelog as "Breaking Changes" instead of "Minor Changes" specially if a previous behaviour is no longer supported. I personally find it helpful to use standardised versioning system like Semantic Versioning and Keep a Changelog.

In general I agree, though we have not yet reached a 1.0 release. I would say at this point every minor update has likely broken someone somewhere. I'm not 100% certain what will lead us to decide that we're at 1.0, but I still see a lot of core things to improve and make more flexible before I would consider Slate stable.

[bryanph]

@KarimNahas The problem is that whenever editor.children gets changed outside of slate's knowledge it has the potential to cause different inconsistencies to happen including the Cannot resolve a DOM point from Slate point error you were talking about or errors with undoing changes that no longer exist. Previously setting the value prop was simply an implicit way of just setting the editor.children value (see below)

slate/packages/slate-react/src/components/slate.tsx

Line 35 in 15f8f86

editor.children = value

I find people generally get confused by the "passing value" API as it seems to indicate that's how you propagate changes, the "reacty" way. But in reality slate operates on operations and keeps its internal state consistent using them. An API that reflects this behavior is therefore desirable was my reasoning.

Also see these two comments: #3575 (comment) and #3575 (comment)

That said I do understand the desire to reset the editor to some particular value. Perhaps we could provide a convenience method for this? Something like Editor.reset(editor)? That way we can guarantee it executes at the right time, avoiding nasty inconsistency problems.

[KarimNahas]

@bryanph I definitely see how, since slate uses operations to mutate its state, using a controlled value pattern would be inconsistent. Specially considering that onChange is triggered for all operations including set_position.

Consequently it think exposing / creating a method that enables the user to safely mutate the editor children would definitely be nice. What about adding a method called resetNodes in NodeTransforms as follows:

export const NodeTransforms: NodeTransforms = {
  ...

  /**
   * resetNodes resets the value of the editor.
   * It should be noted that passing the `at` parameter may cause a "Cannot resolve a DOM point from Slate point" error. 
   */

  resetNodes<T extends Node>(
    editor: Editor,
    options: {
      nodes?: Node | Node[],
      at?: Location
    } = {}
  ): void {
    const children = [...editor.children]
    for (let i = 0; i < children.length; i++) {
      const node = children[i]
      editor.apply({ type: 'remove_node', path: [0], node })
    }

    if (options.nodes) {
      const nodes = Node.isNode(options.nodes) ? [options.nodes] : options.nodes
      for (let i = 0; i < nodes.length; i++) {
        editor.apply({ type: 'insert_node', path: [i], node: nodes[i] })
      }
    }

    const point = options.at && Point.isPoint(options.at) 
      ? options.at 
      : Editor.end(editor, [])
    if (point) Transforms.select(editor, point)
  }

  ...
}

Also, I believe that renaming value to initialValue could help prevent confusion around how to use slate.

What are your thoughts ?

[KarimNahas]

@dylans TL;DR I think that slate should be bumped to version v1.0 on the next breaking change. Note that this PR could have been a good candidate.

I understand that slate is relatively new and is still prone to changes. Having said this, I do believe it has reached a certain level of maturity / stability - I've been using slate for over a year now, and I don't recall facing any breaking changes up until version @slate-react-v0.67.0.

My recommendation is to try and define what does stable mean for for slate. I personally think that if the majority of recent version updates are non-breaking then it is stable. Going through the changelog I feel that most of the recent updates are actually either fixes, improvements of feature addition rather than breaking changes.

Also, I don't think that a library should reach its full maturity / potential before getting bumped to a v1.0.0 release. Most libraries are continuously worked on and having a high major version number is never an issue. react is a great example of that whereby they are now working on version v18.0 and I'm sure that their first couple of versions had a lot of core things to improve and make more flexible.

Lastly, on a personal note, I've been working on a project for about 2 years now. I used to be quite protective of the version number and stayed on v0.x for a very long time. Bumping to v1 was actually very liberating: I no longer had any constraints towards version numbers and could freely use the full spectrum of semantic versioning. The project still has a lot of room for improvement and is far from stable but it is stable enough.

[philicious]

I also think the documentation could need some update regarding how to update the editors value.

I had a rather common use-case: async loading contents from my backend. Therefore couldnt set it as init value in useState. (docs only talk about synchronously loading from a local storage database.)
I spent quite some frustrating time playing around, reading code and docs and eventually stumbling over this discussion.

Docs left the impression to me that you can use value={content} together with useState to update the editors contents any time.

[bryanph]

@philicious agreed about the docs, although for async loading you can simply defer rendering the editor until the content is fetched.

[bsides]

@philicious agreed about the docs, although for async loading you can simply defer rendering the editor until the content is fetched.

Works for the first load of the editor, but what if you want to return to the original state of the editor after changing the values?

For example:

• The UI has a form with an Edit button in the end.
• If the user clicks the Edit button, the UI hides it and then shows a Cancel and Save button.
• The initial value is loaded from a database and then the Slate component is rendered in the first time the page mounts, as suggested above.
• The user then changes the value of the slate textarea.
• Then the user press cancel.
• The expected behavior was to use the initial value that came from the database but it never happens because we lost it and we couldn't change the state externally... again.

How do you tackle this problem? Is it still with editor.children?

EDIT: I'm failing to see this at the documentation even months after. Am I missing something?

[philicious]

@bsides I would (try to) save the original content in state, then use editor.children = originalContent in Cancel button click handler

[bsides]

@bsides I would (try to) save the original content in state, then use editor.children = originalContent in Cancel button click handler

Since the original content is external, that's saving props as state, which is an anti pattern. I'm updating to editor.children for now but considering a better alternative.

[philicious]

@bsides ye in case you consider the content being updated from other places, you need to fetch it again and not use state. or use state + employ mutex locking in your content DB to only allow single edit instance

[bsides]

The thing is, this is a pretty trivial thing for a form to do: to be trully controllable in case we have external data being pulled/pushed asynchronously. Right now it's really not a good implementation because it's trying to do 2 things and failing in both.

[lraccomando]

Would like to +1 on the need to making this clearer in the documentation. For me anyway this is a very common use-case and directly setting editor.children is relatively unintuitive given the design patterns used in other React inputs. Not at all the end of the world, but an explicit note and example in the documentation could save a lot of headaches.

[macrozone]

Is there a chance that this can be reverted? I think it was never justified in the first place, but seeing the problems it created just confirms this.

[onzag]

First of all, I want to say thanks to @ianstormtaylor and @bryanph for maintaining and continuing to build a great library.

Regarding these changes, I wonder whether they actually fix the issue mentioned in #3575: slate used to offer an API similar to any generic input, whereby it exposed props such as value and onChange. This enabled users to interact and maintain the value of the input with a useState hook, as below:

const Editor = () => {
  const [value, setValue] = useState('')
  return (
    <input value={value} onChange={(e) => setValue(e.target.value)} />
  )
}

Consequently, these changes entailed that to be able to interact with the value outside of slate the code would need to be changed from this:

const Editor = () => {
  const editor = useMemo(() => withReact(createEditor())), [])
  const [value, setValue] = useState('')
  return (
    <Slate
       editor={editor}
       value={value}
       onChange={setValue}
    >
      ...
    </Slate>
  )
}

to this:

const Editor = () => {
  const editor = useMemo(() => withReact(createEditor())), [])
  const [value, setValue] = useState('')
  editor.children = value
  return (
    <Slate
       editor={editor}
       value={value}
       onChange={setValue}
    >
      ...
    </Slate>
  )
}

Note the need to manually set the value of editor.children, which if I'm not mistaken can cause a Cannot resolve a DOM point from Slate point. error.

On a separate note, I think changes to the API should be added to the changelog as "Breaking Changes" instead of "Minor Changes" specially if a previous behaviour is no longer supported. I personally find it helpful to use standardised versioning system like Semantic Versioning and Keep a Changelog.

Thanks!

I am having issues with this solution now that I updated to 0.9 due to trying to fix another bug.

This solution causes a lot of side effects on 0.9 even when it used to work previously.

For example when I use Transform.setNodes now it won't update because it will trigger render with the old value, however with keypresses it will register, and only sometimes.

I think now editor.children have to be manually set every time, so while this solution may work some of the time it will not work all of the time, not anymore.

[mrcoles]

Just dropping a note for anyone using slate-history… ([email protected], [email protected])

Setting editor.children doesn’t save the change to your history, e.g., when your editor is this:

const editor = useMemo(() => withHistory(withReact(createEditor())), []);

I see in @KarimNahas example #4540 (comment) that we can instead use editor.apply to make changes that will be remembered.

I ran into an additional problem getting the cursor focus to remain on undo though. I tried adding Transforms.select and also Transforms.setSelection and I was seeing the selectionBefore value in editor.history.undos[0], but when I did undo, the cursor focus would get lost. As a workaround, instead of the node operations, I used "remove_text" and "insert_text" operations and that seems to work (also, I’m using Element.isElement to parse editor.children safely).

[SantosOMartinez]

Hey there Slate community, just want to share my recent attempt at implementing this feature. Where changing the value prop updates the content shown on the editor.

const resetEditor = <T extends Node = Node>(editor: Editor, nodes?: T | T[]) => {
  const { selection } = editor;

  editor.removeNodes({ at: { anchor: editor.start([]), focus: editor.end([]) }});

  if(nodes) editor.insertNodes(Node.isNode(nodes) ? [nodes] : nodes);

  editor.select(selection ?? editor.end([]));
};

Rough example of how you could use it:

const Editor = ({ value, ...props }) => {
  const editor = useMemo(() => createEditor(), []);

  useEffect(() => {
  // Extra logic...

    resetEditor(editor, value); // Where the magic happens!
  }, [value]);

// Rest of the editor code.

Where value is the prop you pass with the slate content that you wish to be controlled.

Hope this helps ✌️!

[xc]

@SantosOMartinez , this is a nice idea, but unfortunately it doesn't work in my case.

My problem:

• using editor.children under root will lose selection
• using useEffect(with editor.children inside) will result in rendering issue(not rendered up-to-date when external changes, always missing on character, so I need to use key for this - that's crazy).

Using the way Santos suggested will result in so many renderings for same content.

Really hope this can be solved. What I'm doing now is a parameter (props:{useEffect?:boolean}) set useEffect and direct editor.children(need to make sure the useEffect:true will be no external change on the content) - kind of ugly...