fix(react): correct previousState for controlled editors

Closes #365
Closes #364

.changeset/olive-rockets-shout.md

@@ -0,0 +1,5 @@
+---
+'@remirror/react': patch
+---
+
+Fix a bug where the previous state was always equal to the updated state for controlled editors. This caused problems with functionality that relies on comparing state values e.g. `PositionerExtension`.

packages/@remirror/core/src/editor-wrapper.ts

@@ -59,6 +59,11 @@ export abstract class EditorWrapper<
    */
   #previousState: EditorState<SchemaFromCombined<Combined>> | undefined;
 
+  /**
+   * A previous state that can be overridden by the framework implementation.
+   */
+  protected previousStateOverride?: EditorState<SchemaFromCombined<Combined>>;
+
   /**
    * True when this is the first render.
    */
@@ -84,7 +89,7 @@ export abstract class EditorWrapper<
   /**
    * The props passed in when creating or updating the `EditorWrapper` instance.
    */
-  protected get props(): Props {
+  get props(): Props {
     return this.#getProps();
   }
 
@@ -94,7 +99,7 @@ export abstract class EditorWrapper<
    * current state will always be equal.
    */
   protected get previousState(): EditorState<SchemaFromCombined<Combined>> {
-    return this.#previousState ?? this.view.state;
+    return this.previousStateOverride ?? this.#previousState ?? this.view.state;
   }
 
   /**
@@ -168,7 +173,7 @@ export abstract class EditorWrapper<
   }
 
   /**
-   * Retrieve the editor state. This is passed through to the extension manager.
+   * Retrieve the editor state.
    */
   protected readonly getState = () => this.view.state;
 

packages/@remirror/react/src/components/__tests__/__snapshots__/react-editor-controlled.spec.tsx.snap

@@ -67,6 +67,22 @@ exports[`Remirror Controlled Component should set the initial value 1`] = `
 </div>
 `;
 
+exports[`Remirror Controlled Component throws when switching from controlled to non-controlled 1`] = `
+"There is a problem with your controlled editor setup.
+
+You have attempted to switch from a controlled to an uncontrolled editor. Once you set up an editor as a controlled editor it must always provide a \`value\` prop.
+
+For more information visit https://remirror.io/docs/errors#rmr0203"
+`;
+
+exports[`Remirror Controlled Component throws when switching from non-controlled to controlled 1`] = `
+"There is a problem with your controlled editor setup.
+
+You have provided a \`value\` prop to an uncontrolled editor. In order to set up your editor as controlled you must provide the \`value\` prop from the very first render.
+
+For more information visit https://remirror.io/docs/errors#rmr0203"
+`;
+
 exports[`Remirror Controlled Component throws when using  \`setContent\` updates 1`] = `
 "There is a problem with your controlled editor setup.
 

packages/@remirror/react/src/components/__tests__/react-editor-controlled.spec.tsx

@@ -33,6 +33,16 @@ function create<Combined extends AnyCombinedUnion>(combined?: Combined[]) {
   };
 }
 
+let errorSpy = jest.spyOn(console, 'error');
+
+beforeEach(() => {
+  errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+});
+
+afterEach(() => {
+  errorSpy.mockRestore();
+});
+
 describe('Remirror Controlled Component', () => {
   it('should set the initial value', () => {
     const { manager, props } = create();
@@ -178,6 +188,68 @@ describe('Remirror Controlled Component', () => {
     expect(() => ctx.clearContent()).toThrowErrorMatchingSnapshot();
   });
 
+  it('throws when switching from controlled to non-controlled', () => {
+    const { manager, props } = create();
+
+    const value = manager.createState({
+      content: '<p>some content</p>',
+      stringHandler: fromHtml,
+    });
+
+    const set = jest.fn();
+
+    const Component = () => {
+      const [state, setState] = useState(value);
+      set.mockImplementation(setState);
+
+      return (
+        <ReactEditor {...props} value={state} manager={manager} onChange={jest.fn()}>
+          {() => <div />}
+        </ReactEditor>
+      );
+    };
+
+    render(<Component />);
+
+    expect(() =>
+      act(() => {
+        set();
+      }),
+    ).toThrowErrorMatchingSnapshot();
+    expect(errorSpy).toHaveBeenCalled();
+  });
+
+  it('throws when switching from non-controlled to controlled', () => {
+    const { manager, props } = create();
+
+    const value = manager.createState({
+      content: '<p>some content</p>',
+      stringHandler: fromHtml,
+    });
+
+    const set = jest.fn();
+
+    const Component = () => {
+      const [state, setState] = useState();
+      set.mockImplementation(setState);
+
+      return (
+        <ReactEditor {...props} value={state} manager={manager} onChange={jest.fn()}>
+          {() => <div />}
+        </ReactEditor>
+      );
+    };
+
+    render(<Component />);
+
+    expect(() =>
+      act(() => {
+        set(value);
+      }),
+    ).toThrowErrorMatchingSnapshot();
+    expect(errorSpy).toHaveBeenCalled();
+  });
+
   it('notifies extensions of state updates via `manager.onStateUpdate`', () => {
     const mock = jest.fn();
 
@@ -222,6 +294,10 @@ describe('Remirror Controlled Component', () => {
     });
 
     expect(mock).toHaveBeenCalledTimes(2);
-    expect(mock.mock.calls[1][0].state).toBe(chain.state);
+
+    const { state, previousState } = mock.mock.calls[1][0];
+
+    expect(state).toBe(chain.state);
+    expect(state).not.toBe(previousState);
   });
 });

packages/@remirror/react/src/components/__tests__/remirror-provider.spec.tsx

@@ -32,6 +32,7 @@ test('`RemirrorProvider`', () => {
 test('multiple `getRootProps` applied to dom throw an error', () => {
   const spy = jest.spyOn(console, 'error').mockImplementation(() => {});
   const manager = createReactManager([]);
+
   const TestComponent: FC = () => {
     const { getRootProps } = useRemirror();
 

packages/@remirror/react/src/components/react-editor.tsx

@@ -69,8 +69,6 @@ export const ReactEditor = <Combined extends AnyCombinedUnion>(
 ) => {
   const { stringHandler = defaultStringHandler, onError, manager, forceEnvironment, value } = props;
 
-  // Cache whether this is a controlled editor.
-  const isControlled = bool(value);
   const { placeholder } = props;
   const isFirstMount = useFirstMountState();
 
@@ -142,21 +140,7 @@ export const ReactEditor = <Combined extends AnyCombinedUnion>(
     methods.onUpdate(previousEditable);
   }, [previousEditable, methods]);
 
-  // Handle controlled editor updates every time the value changes.
-  useEffect(() => {
-    invariant((value && isControlled) || (isNullOrUndefined(value) && !isControlled), {
-      code: ErrorConstant.REACT_CONTROLLED,
-      message: isControlled
-        ? 'You have attempted to switch from a controlled to an uncontrolled editor. Once you set up an editor as a controlled editor it must always provide a `value` prop.'
-        : 'You have provided a `value` prop to an uncontrolled editor. In order to set up your editor as controlled you must provide the `value` prop from the very first render.',
-    });
-
-    if (!value) {
-      return;
-    }
-
-    methods.updateControlledState(value);
-  }, [isControlled, value, methods]);
+  useControlledEditor(methods);
 
   // Return the rendered component
   return methods.render();
@@ -366,6 +350,10 @@ class ReactEditorWrapper<Combined extends AnyCombinedUnion> extends EditorWrappe
           'Controlled editors do not support `clearContent` or `setContent` where `triggerChange` is `true`. Update the `value` prop instead.',
       });
 
+      if (!this.previousStateOverride) {
+        this.previousStateOverride = this.getState();
+      }
+
       this.onChange({ state, tr });
       return;
     }
@@ -385,9 +373,16 @@ class ReactEditorWrapper<Combined extends AnyCombinedUnion> extends EditorWrappe
    * Update the controlled state when the value changes and notify the extension
    * of this update.
    */
-  updateControlledState(state: EditorState<SchemaFromCombined<Combined>>) {
+  updateControlledState(
+    state: EditorState<SchemaFromCombined<Combined>>,
+    previousState?: EditorState<SchemaFromCombined<Combined>>,
+  ) {
+    this.previousStateOverride = previousState;
+
     this.view.updateState(state);
     this.manager.onStateUpdate({ previousState: this.previousState, state });
+
+    this.previousStateOverride = undefined;
   }
 
   /**
@@ -590,3 +585,35 @@ function defaultStringHandler(): never {
       'No valid string handler. In order to pass in `string` as `initialContent` to the remirror editor you must provide a valid stringHandler prop',
   });
 }
+
+/**
+ * A hook which manages the controlled updates for the editor.
+ */
+function useControlledEditor<Combined extends AnyCombinedUnion>(
+  methods: ReactEditorWrapper<Combined>,
+) {
+  const { value } = methods.props;
+
+  // Cache whether this is a controlled editor.
+  const isControlled = useRef(bool(value)).current;
+  const previousValue = usePrevious(value);
+
+  // Handle controlled editor updates every time the value changes.
+  useEffect(() => {
+    // Check if the update is valid based on current value.
+    const validUpdate = value ? isControlled === true : isControlled === false;
+
+    invariant(validUpdate, {
+      code: ErrorConstant.REACT_CONTROLLED,
+      message: isControlled
+        ? 'You have attempted to switch from a controlled to an uncontrolled editor. Once you set up an editor as a controlled editor it must always provide a `value` prop.'
+        : 'You have provided a `value` prop to an uncontrolled editor. In order to set up your editor as controlled you must provide the `value` prop from the very first render.',
+    });
+
+    if (!value) {
+      return;
+    }
+
+    methods.updateControlledState(value, previousValue ?? undefined);
+  }, [isControlled, value, methods, previousValue]);
+}

packages/@remirror/react/src/hooks/editor-hooks.ts

@@ -335,7 +335,7 @@ export function useManager<Combined extends AnyCombinedUnion>(
 
   useEffect(() => {
     return manager.addHandler('destroy', () => {
-      setManager(nextManager);
+      setManager(nextManager.clone());
     });
   }, [manager, nextManager]);