Coder Social home page Coder Social logo

Comments (19)

etrepum avatar etrepum commented on June 3, 2024 2

@StyleT yes, I think it's probably already an issue for next.js that we have had reports about but I think people just did their workarounds (disabling strict mode probably) and left it alone. Currently looking at a fix, I think it is probably as simple as switching to useState, should have a PR to start testing today

from lexical.

acusti avatar acusti commented on June 3, 2024

note that i believe this isn’t specific to changes to the editor’s editable but is actually a more fundamental issue where a number of the editor closure utilities don’t work as expected. for example:

import { $selectAll } from 'lexical';
// …
editor.update(() => {
   $selectAll();
});

doesn’t work using the react v19 beta but does work using v18.3.1. however, toolbars that rely on const [editor] = useLexicalComposerContext(); to apply changes to the editor do work in the v19 beta.

from lexical.

etrepum avatar etrepum commented on June 3, 2024

I suspect what's happening here is due to some behavior change in React.StrictMode and the order of effects since it runs them all twice. If you build it and preview it works. In your listener it's not registering for updates during an effect so it's not subject to the double-running.

from lexical.

etrepum avatar etrepum commented on June 3, 2024

To be clear what I think is happening is this:

  1. Effect A.dev is called, registering an editable listener
  2. Effect A.dev listener is unregistered
  3. Effect B.dev is called, setting the editor state, triggering any editable listeners (but not A.dev because it already unregistered)
  4. Effect A.prod is called, registering an editable listener
  5. Effect B.prod is called, setting the editor state, NEVER triggering any editable listeners because the editor state already matches due to B.dev

I suspect the order of 2. and 3. are probably swapped in React <19

from lexical.

acusti avatar acusti commented on June 3, 2024

@etrepum that seems plausible to me. however, i just re-read the react 19 beta upgrade guide and didn’t see any mention of changes to the order of effect / effect cleanup invocation. they do mention “StrictMode Changes”, but it doesn’t really seem related to me.

When double rendering in Strict Mode in development, useMemo and useCallback will reuse the memoized results from the first render during the second render. Components that are already Strict Mode compatible should not notice a difference in behavior.

As with all Strict Mode behaviors, these features are designed to proactively surface bugs in your components during development so you can fix them before they are shipped to production. For example, during development, Strict Mode will double-invoke ref callback functions on initial mount, to simulate what happens when a mounted component is replaced by a Suspense fallback.

also, do you think i should open a bug on facebook/react repo about this?

from lexical.

etrepum avatar etrepum commented on June 3, 2024

Probably, I guess? If this is indeed the case I'm sure it would break other code that is otherwise correct as well

from lexical.

etrepum avatar etrepum commented on June 3, 2024

Okay looking closer at the React changes and the bug it's definitely a StrictMode issue and it's caused by the change that you referenced:

useMemo and useCallback will reuse the memoized results from the first render during the second render

Previously, separate LexicalEditor instances would be created for first and second renders, but now they are sharing the same LexicalEditor so chaos ensues. Perhaps a workaround that could happen in the core would be to change the 'unsafe' x = useMemo(fn) case(s) to [x] = useState(fn). Other than disabling strict mode, or not using React 19, I'm not sure what you could do to sort this out until some workaround is in place in @lexical/react.

from lexical.

StyleT avatar StyleT commented on June 3, 2024

Mmmm.... Sounds like a storm that's coming... @etrepum do you think it's gonna be a big issue when React 19 comes out of beta?

from lexical.

etrepum avatar etrepum commented on June 3, 2024

I think the example code is also subtly wrong here with regard to effects and StrictMode because of the editorRef is being passed around with a Ref and the effect does not depend on the editor, so if the editor changes then the effect doesn't run again.

from lexical.

etrepum avatar etrepum commented on June 3, 2024

I think we can fix the issue of people using the function version of editorState in this way, but any plug-in that mutates the editor in a way that is not undone by its cleanup function is going to be problematic, because those children component are going to be running their effects twice with the same editor. Not much we can do about that other than warn people about this classic trap.

const [editor] = useLexicalComposerContext();
useEffect(() => {
  // this happens twice in StrictMode… which is expected, but problematic if this is a mutation
}, [editor]);

from lexical.

acusti avatar acusti commented on June 3, 2024

I think the example code is also subtly wrong here with regard to effects and StrictMode because of the editorRef is being passed around with a Ref and the effect does not depend on the editor, so if the editor changes then the effect doesn't run again.

i based the example on code examples i found of initialConfig usage where the editorState callback was used to capture a ref to the editor instance, so i assumed the editor instance was stable for the lifetime of the LexicalComposer component. @etrepum is that not a safe assumption? if so, is there a way to include logic in the component that operates on the editor instance, or does all such logic need to move into a child component that can then use const [editor] = useLexicalComposerContext()?

update i modified the code example to introduce EditablePlugin.tsx, which takes props.isEditable and updates the editor in a useEffect:

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();

    useEffect(() => {
        editor.setEditable(isEditable);
    }, [editor, isEditable]);

    return null;
}

from lexical.

etrepum avatar etrepum commented on June 3, 2024

All logic in a react app should be using const [editor] = useLexicalComposerContext(), there's an EditorRefPlugin for that purpose. Is that example in the docs or the repo somewhere?

from lexical.

acusti avatar acusti commented on June 3, 2024

@etrepum couldn’t find the example digging through my browser history just now but i will let you know if i find it!

from lexical.

acusti avatar acusti commented on June 3, 2024

@etrepum just saw your comment closing #6041:

After thinking about this some more I think the approach we have is probably correct enough

i updated my code example to use the const [editor] = useLexicalComposerContext() approach (see EditablePlugin.tsx), and the example is still quite broken with the react v19 beta (editor.setEditable() does nothing, editor.update(() => { $selectAll(); }) has no effect, probably other things…). consider that, it seems to me that the approach isn’t correct enough yet. am i misunderstanding your comment?

from lexical.

etrepum avatar etrepum commented on June 3, 2024

Can you describe exactly what the problem is with your current example? If you click on it, it becomes editable, although a second click is required to give the editor focus with the way that this example is implemented.

Selection does not reconcile to the DOM if the editor is not editable, which is probably the reason why you think that is broken.

from lexical.

acusti avatar acusti commented on June 3, 2024

@etrepum apologies for the inaccurate comment, i hadn’t realized that the contenteditable HTML attribute is now updating. i’ve been trying to keep my actual reproduction in my own app and the minimal reproducible demo in sync but i hadn’t restored the focus logic to the minimal repro since the last refactor. thanks for your assistance with figuring out the core issue!


fwiw, i added the editor focus / $selectAll logic to EditablePlugin.tsx and no matter where i attempted to apply it in the editor lifecycle, i couldn’t get it to work. i tried invoking editor.focus() immediately after calling editor.setEditable(true), i tried putting it in a editor.update(() => { editor.focus() }) closure, and i tried putting it in a registerEditableListener. here’s what that looked like:

import { useLexicalComposerContext } from '@lexical/react/LexicalComposerContext';
import { $selectAll } from 'lexical';
import { useEffect } from 'react';

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();

    useEffect(() =>
        editor.registerEditableListener((_isEditable) => {
            if (_isEditable) {
                editor.focus();
                editor.update(() => {
                    // editor.focus(); // also tried focussing here with no luck
                    $selectAll();
                });
            }
        }),
        [editor],
    );

    useEffect(() => {
        editor.setEditable(isEditable);
        /* I first tried to focus from here, but it didn’t work, so I tried the listener
        if (isEditable) {
            editor.focus();
            editor.update(() => {
                // editor.focus(); // also tried focussing here with no luck
                $selectAll();
            });
        }
        */
    }, [editor, isEditable]);

    return null;
}

i did wind up getting it to work, but it’s a classic “this seems wrong and fragile” solution, so i’m not delighted. i had to wrap the editor.focus() invocation in a setTimeout. is there a better way?

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();

    useEffect(() =>
        editor.registerEditableListener((_isEditable) => {
            if (_isEditable) {
                setTimeout(() => {
                    editor.focus();
                    editor.update(() => {
                        $selectAll();
                    });
                }, 0); // next tick
            }
        }),
        [editor],
    );

    useEffect(() => {
        editor.setEditable(isEditable);
    }, [editor, isEditable]);

    return null;
}

from lexical.

etrepum avatar etrepum commented on June 3, 2024

I think your bug boils down to the fact that the editor is already editable before the listener is attached because it was marked editable during the first run of the effect in StrictMode, so the listener isn't triggered. An easier approach to get correct in StrictMode is demonstrated by the useLexicalEditable() hook and you could trigger an effect based on that without manually registering any listeners.

from lexical.

etrepum avatar etrepum commented on June 3, 2024

EditableState.tsx

import { useLexicalComposerContext } from '@lexical/react/LexicalComposerContext';
import useLexicalEditable from '@lexical/react/useLexicalEditable';
import { $getSelection, $selectAll } from 'lexical';
import { useEffect } from 'react';

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();
    const editableState = useLexicalEditable();
    useEffect(() => {
        editor.setEditable(isEditable);
    }, [editor, isEditable]);
    useEffect(() => {
        if (editableState) {
            // Note this is basically just editor.focus() but
            // with a different default selection. If you want
            // to select the beginning or end just use that.
            editor.update(() => {
                const selection = $getSelection();
                if (selection) {
                    // Force selection to be reconciled
                    selection.dirty = true;
                } else {
                    $selectAll();
                }
            });
        }
    }, [editor, editableState]);

    return null;
}

from lexical.

acusti avatar acusti commented on June 3, 2024

@etrepum that works great, thanks for the followup!

I think your bug boils down to the fact that the editor is already editable before the listener is attached because it was marked editable during the first run of the effect in StrictMode, so the listener isn't triggered

i don’t think that’s the case, because i’m defaulting the editor to not editable, so the editor.focus() / $selectAll() code isn’t executing at all during initial render(s) and is only triggered after all listeners have been attached and the user clicks on the editor to start interacting with it. instead, it seems like the editable listener is being triggered before the editor has actually become editable (or the changes have been updated to / reconciled with the DOM). with the useEffect-based approach where editableState is in the dependency array (and with my setTimeout hack), on the other hand, the logic is triggered after the react render lifecycle has completed with the editable state change, so manipulating the selection succeeds. (just my not well-informed speculation, of course!)

either way, thanks again for all your help with this.

from lexical.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.