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

Only use Slate Provider's value prop as initial state #4540

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

bryanph
Copy link
Contributor

@bryanph bryanph commented Sep 20, 2021

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
Copy link

changeset-bot bot commented Sep 20, 2021

🦋 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

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

bryanph commented Sep 28, 2021

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

@dylans dylans merged commit 11ef83b into ianstormtaylor:main Oct 12, 2021
jaked added a commit to jaked/slate-explorer that referenced this pull request Oct 19, 2021
jaked added a commit to jaked/programmable-matter that referenced this pull request Oct 19, 2021
@Ghost---Shadow
Copy link

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

@Ghost---Shadow
Copy link

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

@hassenc
Copy link

hassenc commented Oct 20, 2021

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
Copy link
Collaborator

dylans commented Oct 20, 2021

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
Copy link

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
Copy link
Contributor Author

bryanph commented Oct 20, 2021

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
Copy link

natqe commented Oct 22, 2021

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
Copy link

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
Copy link
Collaborator

dylans commented Oct 25, 2021

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
Copy link
Contributor Author

bryanph commented Oct 26, 2021

@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)

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
Copy link

@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
Copy link

@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
Copy link

philicious commented Nov 27, 2021

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
Copy link
Contributor Author

bryanph commented Nov 29, 2021

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

tiberiuichim added a commit to eea/volto-slate that referenced this pull request Nov 30, 2021
* Refs #142010 - Optimize Volto-addons gitflow pipelines

* Revert to older version of slate-react

without the breaking API change

* Upgrade slate and slate-react versions

which brings up the problem with the breaking API change in Slate-React:
ianstormtaylor/slate#4540

* Split text block editor components in separate modules

* [JENKINS] Fix eslint

* Make it work with new react-slate method of getting the value

* Multiple Description blocks on the same page update each other

In fact, does the changes to the SlateEditor component for the
Slate-based Title & Description blocks too.

* Remove useless commented line

* Backwards block merge 2 (#195)

* Solve bug when merging blocks with Backspace

* Remove some dead code

* Update Jest snapshots

* Update Jest snapshots

Co-authored-by: Silviu Bogan <silviubogan@gmail.com>

* Automated release 5.1.2

Co-authored-by: valentinab25 <valentinab25>
Co-authored-by: Silviu Bogan <silviubogan@gmail.com>
Co-authored-by: Alin Voinea <contact@avoinea.com>
Co-authored-by: EEA Jenkins <@users.noreply.github.com>
tiberiuichim added a commit to eea/volto-slate that referenced this pull request Dec 5, 2021
* Refs #142010 - Optimize Volto-addons gitflow pipelines

* Revert to older version of slate-react

without the breaking API change

* Upgrade slate and slate-react versions

which brings up the problem with the breaking API change in Slate-React:
ianstormtaylor/slate#4540

* Split text block editor components in separate modules

* [JENKINS] Fix eslint

* Make it work with new react-slate method of getting the value

* Multiple Description blocks on the same page update each other

In fact, does the changes to the SlateEditor component for the
Slate-based Title & Description blocks too.

* Remove useless commented line

* Backwards block merge 2 (#195)

* Solve bug when merging blocks with Backspace

* Remove some dead code

* Update Jest snapshots

* Update Jest snapshots

Co-authored-by: Silviu Bogan <silviubogan@gmail.com>

* Automated release 5.1.2

* [JENKINS] Refs #142010 - Optimize Volto-addons gitflow pipelines

* Automated release 5.1.3

Co-authored-by: valentinab25 <valentinab25>
Co-authored-by: Silviu Bogan <silviubogan@gmail.com>
Co-authored-by: Tiberiu Ichim <tiberiu.ichim@gmail.com>
Co-authored-by: Alin Voinea <contact@avoinea.com>
Co-authored-by: Tiberiu Ichim <tiberiuichim@users.noreply.github.com>
Co-authored-by: EEA Jenkins <@users.noreply.github.com>
notbakaneko added a commit to notbakaneko/osu-web that referenced this pull request Jun 27, 2022
@bsides
Copy link

bsides commented Oct 11, 2022

@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
Copy link

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

@bsides
Copy link

bsides commented Oct 11, 2022

@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
Copy link

@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
Copy link

bsides commented Oct 20, 2022

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
Copy link

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
Copy link
Contributor

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
Copy link

onzag commented Feb 9, 2023

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
Copy link

mrcoles commented Aug 10, 2023

Just dropping a note for anyone using slate-history… (slate@0.94.1, slate-history@0.94.0)

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
Copy link

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
Copy link

xc commented Oct 21, 2024

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delayed setState(value) results in Cannot resolve a DOM point from Slate point