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

fix: yjs collaboration plugin in react strict mode #6271

Merged

Conversation

meronogbai
Copy link
Contributor

@meronogbai meronogbai commented Jun 7, 2024

Description

The collaboration plugin doesn't work well in nextjs 14.2 with strict mode enabled. See videos below to see how it breaks. I fixed the issues by relying on a useEffect instead of a useMemo to initialize yjs binding and provider.

Relevant thread

https://discord.com/channels/953974421008293909/1233102329931366531/1233102329931366531

Test plan

  • Clone test repo
  • cd into the cloned test repo
  • Sign up for liveblocks and add LIVEBLOCKS_SECRET_KEY to the root .env.local file. Alternatively, I can send you a secret key as well.
  • Run npm run dev and test the editor on multiple tabs/windows/browsers.
  • To fix the issue, copy the updated LexicalCollaborationPlugin file into node_modules
cp LexicalCollaborationPlugin.dev.mjs node_modules/@lexical/react/LexicalCollaborationPlugin.dev.mjs
  • Stop the previous development server and start it again. Keep in mind that you have to remove the .next folder because nextjs caches built modules.
rm -r .next && npm run dev

Before

https://www.loom.com/share/460727ea35b04e26a311d55d5283be3

After

https://www.loom.com/share/d3a2701d4ef6456a9bd7c9a202d38ebe

Note

I tried to run npm run build-release, but it didn't work due to #5420 so I had to compile "manually". Never mind, it was just an error with my changes.

Copy link

vercel bot commented Jun 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2024 10:31pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2024 10:31pm

@facebook-github-bot
Copy link
Contributor

Hi @meronogbai!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

Copy link

github-actions bot commented Jun 7, 2024

size-limit report 📦

Path Size
lexical - cjs 28.31 KB (0%)
lexical - esm 28.13 KB (0%)
@lexical/rich-text - cjs 36.77 KB (0%)
@lexical/rich-text - esm 28.09 KB (0%)
@lexical/plain-text - cjs 35.36 KB (0%)
@lexical/plain-text - esm 25.33 KB (0%)
@lexical/react - cjs 38.51 KB (0%)
@lexical/react - esm 29.14 KB (0%)

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 7, 2024
@meronogbai meronogbai marked this pull request as ready for review June 7, 2024 20:21
Copy link
Contributor

@StyleT StyleT left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for the fix but npm run tsc is failing. Can you pls fix it?

@fantactuka
Copy link
Contributor

Could you use git mv for renaming to retain file history

@meronogbai
Copy link
Contributor Author

meronogbai commented Jun 7, 2024

@fantactuka I believe I did that already. I renamed LexicalCollaborationPlugin.ts to LexicalCollaborationPlugin.tsx in the first commit and then added my changes to LexicalCollaborationPlugin.tsx in the second commit.

image

The blame info hasn't changed as you can see in https://github.com/facebook/lexical/blame/e6982e727b59eddaf69b25ba8fd32a9e837d969b/packages/lexical-react/src/LexicalCollaborationPlugin.tsx

@StyleT
Copy link
Contributor

StyleT commented Jun 10, 2024

@meronogbai Hi! Took another look at this.. So we still call providerFactory here twice, although we now do proper destruction handling, which is great! However I would say we still may do 1 step forward and make this code actually call providerFactory only once.

Why? From my experience y-webrtc would throw on you even if you call provider.disconnect().

Here is simple test that you can add to this PR and make it pass it:

/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */

import { CollaborationPlugin } from '@lexical/react/LexicalCollaborationPlugin';
import { LexicalComposer } from '@lexical/react/LexicalComposer';
import { ContentEditable } from '@lexical/react/LexicalContentEditable';
import {LexicalErrorBoundary} from '@lexical/react/LexicalErrorBoundary';
import { RichTextPlugin } from '@lexical/react/LexicalRichTextPlugin';
import * as React from 'react';
import {createRoot, Root} from 'react-dom/client';
import * as ReactTestUtils from 'shared/react-test-utils';
import * as Y from 'yjs';

describe(`LexicalCollaborationPlugin`, () => {
  let container: HTMLDivElement;
  let reactRoot: Root;

  const editorConfig = Object.freeze({
    // NOTE: This is critical for collaboration plugin to set editor state to null. It
    // would indicate that the editor should not try to set any default state
    // (not even empty one), and let collaboration plugin do it instead
    editorState: null,
    namespace: 'Test editor',
    nodes: [],
    // Handling of errors during update
    onError(error: Error) {
      throw error;
    },
  });

  beforeEach(() => {
    container = document.createElement('div');
    reactRoot = createRoot(container);
    document.body.appendChild(container);
  });

  test(`providerFactory called only once`, () => {
    const providerFactory = jest
      .fn((id: string, yjsDocMap: Map<string, Y.Doc>) => {
        const doc = new Y.Doc();
        yjsDocMap.set(id, doc);

        return {
          awareness: {
            getLocalState: () => null,
            getStates: () => new Map(),
            off: () => {},
            on: () => {},
            setLocalState: () => {},
          }, 
          connect: () => {}, 
          disconnect: () => {},
          off: () => {},
          on: () => {},
        }
      });
    function MemoComponent() {
      return (<LexicalComposer initialConfig={editorConfig}>
        {/* With CollaborationPlugin - we MUST NOT use @lexical/react/LexicalHistoryPlugin */}
        <CollaborationPlugin
          id="lexical/react-rich-collab"
          providerFactory={providerFactory}
          // Unless you have a way to avoid race condition between 2+ users trying to do bootstrap simultaneously
          // you should never try to bootstrap on client. It's better to perform bootstrap within Yjs server.
          shouldBootstrap={false}
        />
        <RichTextPlugin
          contentEditable={<ContentEditable className="editor-input" />}
          placeholder={<div className="editor-placeholder">Enter some rich text...</div>}
          ErrorBoundary={LexicalErrorBoundary}
        />
      </LexicalComposer>);
    }
    ReactTestUtils.act(() => {
      reactRoot.render(
        <React.StrictMode>
          <MemoComponent />
        </React.StrictMode>,
      );
    });
    
    expect(providerFactory).toHaveBeenCalledTimes(1);
  });
});

How to make code pass this test? Use "dirty" hack from here https://taig.medium.com/prevent-react-from-triggering-useeffect-twice-307a475714d7

@meronogbai
Copy link
Contributor Author

Thank you so much @StyleT! I pushed a commit with your suggested changes 🫡

@StyleT
Copy link
Contributor

StyleT commented Jun 11, 2024

I'm good to merge this, seems like test failures are unrelated

@Sahejkm Sahejkm added this pull request to the merge queue Jun 12, 2024
Merged via the queue into facebook:main with commit 6a45fc3 Jun 12, 2024
38 of 39 checks passed
@meronogbai meronogbai deleted the fix/yjs-collaboration-react-strict-mode branch June 12, 2024 01:18
@nimeshnayaju
Copy link

Thanks for fixing this! Would it be possible to release a 0.16.1 version with this bug fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants