ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.33k stars 3.22k forks source link

Bug: Cannot update slate state externally #4612

Open Ghost---Shadow opened 2 years ago

Ghost---Shadow commented 2 years ago

Description

The PR https://github.com/ianstormtaylor/slate/pull/4540 removed the ability to update the slate state using the value prop. As a result, we cannot inject externally changed state anymore.

If there is another way to update the state externally, then the documentation should be updated as well. https://docs.slatejs.org/walkthroughs/06-saving-to-a-database

Environment

circlingthesun commented 2 years ago

Ouch, we got bitten hard by this. Downgraded for now.

circlingthesun commented 2 years ago

@Ghost---Shadow if you look at what was changed (https://github.com/ianstormtaylor/slate/pull/4540/files), you can easily get the same behaviour by setting a key property on the Slate element and incrementing whenever your state changes.

aboveyunhai commented 2 years ago

@Ghost---Shadow if you look at what was changed (https://github.com/ianstormtaylor/slate/pull/4540/files), you can easily get the same behaviour by setting a key property on the Slate element and incrementing whenever your state changes.

Can you elaborate a little bit more with the key hook code sample. Wouldn't it cause some re-render issues of entire slate component while changing the value or visually the text. I'm ok with the change but little bit lose the direction to achieve the similar behavior in a proper way. Just roll back to last version until the doc fixed,

I am very aware of any slate "breaking" change but it's a still a powerful blackbox machine to me, still on basic/user level and try to understand the internal logic.

ivan-nemtinov commented 2 years ago

A temporary solution: update editor.children instead of value, as specified in slate-react@0.67.0 release notes.

Ghost---Shadow commented 2 years ago

This is a hack, not a solution. If this is the official way of updating the state, then the documentation should be updated as well.

ivan-nemtinov commented 2 years ago

It works, but I agree, it's a hack. Setting editor.children directly also requires manual fixing of selection, because selected range remains the same even if some new nodes are added above the selection.

aboveyunhai commented 2 years ago

It works, but I agree, it's a hack. Setting editor.children directly also requires manual fixing of selection, because selected range remains the same even if some new nodes are added above the selection.

It does not seem like a hack based on the PR discussion. It's just not a React way, and not necessary to be a React way. but selection is definitively a problem. And it's confusing comparing to the past implementation.

jan-martinek commented 2 years ago

It took me several days to find out that changes in #3216 were silently reversed in #4540. It would be great to reflect the changes not only in docs as others say above, but also in API (eg revert value to defaultValue or initialValue) as the value prop implies controlled behavior.

It‘s possible that out there are many others struggling to find out what went wrong (in our case it wasn’t an upgrade going bad — that would make me go for changelists sooner, but documentation being out of date and API pretending that everything is okay).

jan-martinek commented 2 years ago

(Hence, I’d propose relabeling this as an improvement and changing the name of the prop — such change would highlight its breaking nature, as the difference between un- and controlled components is night and day.)

Pines-Cheng commented 2 years ago

A solution: update editor.children instead of value, as specified in slate-react@0.67.0 release notes.

It's really not a react way.

ivan-nemtinov commented 2 years ago

It's really not a react way.

Of course it's not. I hope somebody will make a refactoring.

aboveyunhai commented 2 years ago

It's really not a react way.

Of course it's not. I hope somebody will make a refactoring.

slate was claimed to be built on top of React in github main page, but we have slate react dedicated for React logic purpose if I'm not mistaken with the major functionalities. So does the same claim applies on slate core(slate)?

Now the problem is that if this PR #4540 is merged intentionally, does it mean the slate core actually does not rely on react? which is extremely important for the future release. as well as my questionable past statement .

It's just not a React way, and not necessary to be a React way. ?

Strictly speaking: controlled component vs uncontrolled component instead of react way. Indeed, there are some silent disagreements contributors were unware of. I guess it's good to ask @bryanph for clarification and his plan.

ivan-nemtinov commented 2 years ago

slate was claimed to be built on top of React in github main page, but we have slate react dedicated for React logic purpose if I'm not mistaken with the major functionalities. So does the same claim applies on slate core(slate)?

Now the problem is that if this PR #4540 is merged intentionally, does it mean the slate core actually does not rely on react? which is extremely important for the future release. as well as my questionable past statement .

Yes, slate core doesn't rely on react by design. slate-react modifies editor.children in react hooks.

Indeed, there are some silent disagreements contributors were unware of. I guess it's good to ask @bryanph for clarification and his plan.

Would be nice if you did it!

bryanph commented 2 years ago

@aboveyunhai slate was redesigned at some point so that the slate package is just for modifying the document and slate-react is for the react specific part (rendering the changes to the DOM). I think you got it right in the controlled component vs uncontrolled component part. I originally argued in an issue that it should be explicitly an uncontrolled component because some people were getting problems with the selection not updating as a result of setting the "controlled" value state, see https://github.com/ianstormtaylor/slate/issues/3575#issuecomment-923227427. I elaborated on this here: https://github.com/ianstormtaylor/slate/pull/4540#issuecomment-951770910.

Note that an Editor.reset method was proposed there which would then also handle the selection resetting and the history resetting (which wasn't handled by passing a different value prop). Will see if I can find some time in the next few weeks to give that a shot (or someone else can give it a shot of course :smile:). As mentioned above setting editor.children explicitly to an initial value is the recommended way for now. I agree that's not the most elegant solution but hopefully an Editor.reset API of some form will resolve that.

mlajtos commented 2 years ago

Changing content via Transform.transform(op) does not reflect changes visually, however the editor.children does change. Reproduction code: https://codesandbox.io/s/great-waterfall-sp2jm?file=/src/App.tsx

@bryanph What to do in this case?

bryanph commented 2 years ago

@mlajtos use editor.apply instead

odusseys commented 2 years ago

We are building a collaborative editing tool and this change is pretty much a killed when it comes to reconciling state from the server. Using a key to fully re-render is destructive and pretty much kills the UX as all selection state is lost. I would strongly consider not exposing an uncontrolled API as this makes it very tough to use properly for any non-basic use case, especially when external state can override application state (which is pretty much all the time in 2022).

odusseys commented 2 years ago

@bryanph do you have an code snippet to explain how to do this ? the documentation for operations is laconic

jmikrut commented 2 years ago

This just caused me to blow about 3 million brain cells. We use Slate extensively in Payload and also need to programmatically handle incoming values. For now, I've done as described above in this thread and added a key based on initialValue to a div containing the Slate editor.

Here is the commit for anyone interested. Obviously Payload's implementation is quite complex but the actual changes here are fairly simple. This works for now but man that threw me for a loop.

spudly commented 2 years ago

For those who just need a quick workaround, here's how I fixed it in my code:

Custom useForceUpdate hook defined elsewhere:

const useForceUpdate = () => {
  const [, setState] = useState<number>(0);

  const forceUpdate = useCallback(() => {
    setState(n => n + 1);
  }, []);

  return forceUpdate;
};

Code inside my component that renders <Slate>:

  const forceUpdate = useForceUpdate();
  useEffect(() => {
    editor.children = value;
    forceUpdate();
  }, [editor, value, forceUpdate]);
franky47 commented 2 years ago

I stumbled upon this issue when connecting the Slate value to a form (using Formik), and trying to reset it on submit.

Here's my "hack", which updates the key only when the value changes to the default. It's not a fully-controlled solution, but it did the trick:

const defaultRichTextValue: Descendant[] = [
  {
    type: 'paragraph',
    children: [{ text: '' }],
  },
]

function useResetKey(value: Descendant[]) {
  const [key, setKey] = React.useState(0)
  React.useEffect(() => {
    if (JSON.stringify(value) === JSON.stringify(defaultRichTextValue)) {
      setKey(n => n + 1)
    }
  }, [value])
  return key
}

const RichTextEditor = ({
  value,
  onChange,
  ...props
}) => {
  const resetKey = useResetKey(value)
  const editor = React.useRef(withHistory(withReact(createEditor())))
  return (
    <Slate
      editor={editor.current}
      value={value}
      onChange={onChange}
      key={resetKey}
    >
      <Editable {...props} />
    </Slate>
  )
}
ArthurBZhou commented 2 years ago

thx! @franky47 Hope this bug will be ruled out as soon as possible!

jmadelaine commented 2 years ago

We are also seeing this as a major blocker to using Slate. We have multiple ways to display comment threads (sidebar and a floating comment overlay), and sometimes both are visible when typing a new comment, meaning they share state. It's pretty fundamental to have the ability to control an input's value externally, and in fact, one way data flow is one of the core pillars of React. This is a bug, and the key hack is just that, a hack.

jonathan-chin commented 1 year ago

this is also a dealbreaker to adoption for me.

gsvh commented 1 year ago

Really loved using Slate until I faced this problem and read this thread. I have been stuck on this for a while, and the solutions mentioned here are way below the standard set by how the rest of Slate works.

gsvh commented 1 year ago

The solution I used doesn't need any useEffects or extra states

<Slate editor={editor} value={value} key={JSON.stringify(value)}>

I stringify the value and use that as the key, so that when the value changes, the key changes and the component is rerendered

Rinzyy commented 1 year ago

I've been finding various options to resolve this issue, and it appears that your solution is the simplest and most efficient method. This should be on the documentation.

hypeJunction commented 1 year ago

Using keys and forcing the component to re-render is a no go: major performance hog and prone to errors with caret position and selection. What are our options here? Any intention to fix this issue, and if so, any particular timeline in sight? I love Slate so far, but this particular issue is making me reconsider. What legacy version can I roll back to to get controlled react components? Also, what's up with versioning here? Why is everything on 0.x.x if there were many breaking changes already? I would be willing to invest time to figure out what's going on in the source code and how we can make things controlled, but I need some indication if someone is already working on it, and if it's something that the core intends to support again - otherwise I will just fork and go.

macrozone commented 1 year ago

I am maintainin https://github.com/react-page/react-page which uses slate for its rich-text editor. Since the infamous change to an uncontrolled component, we suffer from bugs and inconsistencies. Setting the value from outside is important for us, because we have our own undo/redo mechanism.

Uncontrolled components are in my opinion an antipattern in most cases and the amount of headache it created alone in this thread here shows why. I recently created another issue here https://github.com/ianstormtaylor/slate/issues/5281 but i think it a duplicate of this here.

I tried different workarounds, but I found none that works consistently. Slate really has to revert this decision, I don't think there is another way.

hypeJunction commented 1 year ago

What we ended up doing to dynamically modify the state of slate, is passing the ref of the editor to the parent components via a Context wrapper, and using Slate API to mutate nodes. Not the most elegant solution, but it did the trick for us. Though, I would really prefer having a controlled input rather than keeping track of editor refs.

ggrantrowberry commented 1 year ago

@hypeJunction Do you have any examples of how you did that that you can share by any chance? I'm trying to control undo and redo from outside of Slate and am running into issues.

hypeJunction commented 1 year ago

@ggrantrowberry Here are some snippets:

Context and utility components defined as such:

import React, { PropsWithChildren, useContext, useEffect, useRef } from 'react'
import { useSlate } from 'slate-react'
import { Editor } from 'slate'

type TemplatedFormContextValue = {
  editors: Map<string, Editor>
}

const TemplatedFormContext = React.createContext<TemplatedFormContextValue | null>(null)

export const TemplatedForm = ({ children }: PropsWithChildren<any>) => {
  const ref = useRef<Map<string, Editor>>()

  if (!ref.current) {
    ref.current = new Map()
  }

  return (
    <TemplatedFormContext.Provider
      value={{
        editors: ref.current
      }}
    >
      {children}
    </TemplatedFormContext.Provider>
  )
}

export const TemplatedFormEditorReference = ({ name }: { name: string }) => {
  const editor = useSlate()
  const ctx = useTemplatedForm()

  useEffect(() => {
    if (ctx?.editors) {
      ctx.editors.set(name, editor)
    }
  }, [editor, name])

  return null
}

export const useTemplatedForm = () => {
  return useContext(TemplatedFormContext)
}

You then add the reference to the context when rendering Slate

export const RichEditor = forwardRef(
  (
    { value, onChange, before, toolbar, after, name, ...rest }: RichEditorProps,
    ref: ForwardedRef<HTMLDivElement>
  ) => {
    const { editor } = useSlateEditor()

    const defaultValue = Array.isArray(value) && value.length > 0 ? value : defaultEditorValue

    return (
      <InputBox
        variant='outlined'
        color='neutral'
        control={
          <Slate value={defaultValue} editor={editor} onChange={onChange}>
            {before}
            <div className={b()}>
              {toolbar}
              {name ? <TemplatedFormEditorReference name={name} /> : null}
              <EditableArea ref={ref} {...rest} />
            </div>
            {after}
          </Slate>
        }
      />
    )
  }
)

And to modify the contents of the editor, you would use slate apis:

import { Transforms } from 'slate'
import { cloneDeep } from 'lodash'

export const TemplatePicker = ({ templates }: InsertTemplateProps) => {
  const ctx = useTemplatedForm()

  const appendTemplate = (fields: { name: string; text: StructuredText[] }[]) => {
    fields.forEach((field) => {
      const editor = ctx?.editors.get(field.name)
      if (editor) {
        editor.children.map((item) => {
          Transforms.delete(editor, { at: [0] })
        })
        field.text.map((node) => {
          Transforms.insertNodes(editor, cloneDeep(node))
        })
      }
    })
  }

   // template selector with callback
}

This could probably be optimized. The code was written by our junior with some initial input from me, and I haven't seen the latest version.

Neczesk commented 1 year ago

I just wanted to chime in, I really wanted to use slate for my project but it became unusable to me because of the lack of a simple way to update the state externally. If I had the time I'd try to come up with a pull request and change it myself, but I don't have the knowledge to do it in a reasonable time frame.

joulev commented 12 months ago

Just to add in, I have to make my editor controllable as well, so I did something like this, using the key idea as described above, but only update the key when the change comes from external sources.

"use client";

import { useMemo, useState } from "react";
import { Descendant, createEditor } from "slate";
import { Editable, Slate, withReact } from "slate-react";

const initialValue: Descendant[] = [
  { type: "paragraph", properties: {}, children: [{ text: "Hello World!" }] },
];

function Editor({
  value,
  onChange,
}: {
  value: Descendant[];
  onChange: (value: Descendant[]) => void;
}) {
  const editor = useMemo(() => withReact(createEditor()), []);
  return (
    <Slate editor={editor} initialValue={value} onChange={onChange}>
      <Editable />
    </Slate>
  );
}

export default function Page() {
  const [value, setValueRaw] = useState(initialValue);
  const [key, setKey] = useState(0);
  function setValue(value: Descendant[]) {
    setValueRaw(value);
    setKey(key + 1);
  }
  return (
    <div>
      <button type="button" onClick={() => setValue(initialValue)}>
        Reset
      </button>
      <Editor key={key} value={value} onChange={setValueRaw} />
    </div>
  );
}

and I just use setValue anywhere I need to update the state outside Slate.

Can't say rerendering the entire editor is good for performance, but this is the only solution applicable to our app; assigning to editor.children looks too un-Reacty and sometimes causes errors.

aboveyunhai commented 11 months ago

@joulev as I came through a lot of uncontrolled components (canvas, dnd, table editing, occasionally input), I hit the limitation of React way on doing render and state control, I sincerely believe Slate (or other text-editors) should belong to the uncontrolled component instead of controlled component due to the optimization, the granular state update, potentially collaborative mode, and a lot of other things that you could only be done via uncontrolled component.

To be more aggressive, doing the React way here is not the correct way of doing React way. 😂 The related original PR (everyone here blamed) that removed the external state binding, finally clicks for me.

Uncontrolled component as well as mutability really sounds like a negative term in the React world, but in fact, we just fallback to the original de facto JavaScript way of handling the DOM, especially for something complex like text-editor. Since this issue continuously and occasionally got picked up by someone. I highly recommended people who read and agree my post, to migrate their implementation to uncontrolled component. If I had time to pick up the Slate code I had written, I will simply change everything based on what I said here.

jlarrubiaq commented 11 months ago

Same problem here, we tried to update from an old version of Slate so we could benefit from the use of plain objects instead on Inmutable but this is preventing us from completing the migration. We might need to look for alternatives if Slate doesn't provide a non-hacky way to control the component value. The solutions above break the editor selection when externally changing the value.

seyhak commented 8 months ago

the fix is in documentation, I found it after few hours ... If you want to update the editor's content in response to events from outside of Slate, you need to change the children property directly. The simplest way is to replace the value of editor.children editor.children = newValue and trigger a re-rendering (e.g. by calling editor.onChange() in the example above). Alternatively, you can use Slate's internal operations to transform the value, for example: https://docs.slatejs.org/walkthroughs/06-saving-to-a-database

cheestudio commented 8 months ago

@seyhak Cannot thank you enough! It's so obvious and simple, yet I was overcomplicating it. It's clear in the docs, but it's also apparently really easy to miss! In my use case, replacing the values and triggering a re-render was two lines and perfect for what I needed.

Kevo-gmm commented 2 months ago

@Ghost---Shadow if you look at what was changed (https://github.com/ianstormtaylor/slate/pull/4540/files), you can easily get the same behaviour by setting a key property on the Slate element and incrementing whenever your state changes.

works perfectly fine