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 inconsistent behavior of useAsyncValue and useCKEditorCloud hooks on Vite and Next runtimes. #541

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Sep 30, 2024

Suggested merge commit message (convention)

Fix: Behavior of useCKEditorCloud hook is now consistent on Vite and Next runtimes while changing properties.
Internal: Fix incorrect typings of useAsyncCallback and useAsyncValue callback arguments types.
Tests: Add missing cleanup methods to few tests.


Additional information

It looks like useRef behavior is inconsistent across React frameworks, and it is not related to React version itself (at least 18.x one, 19.x is not affected at all).

Let's assume this component that is being rendered in strict mode:

const Component = () => {
    const ref = useRef(null);

    console.info(ref.current);

    ref.current = 'test';

    return null;
};
  1. On Vite it will print null two times.
  2. On Next.js it will print null the first time, and then test second time.

It looks like something changed somewhere in React world and second approach is no longer consistent across frameworks. At the time of creating useInstantEffect it was. It affects our useCKEditorCloud which uses useInstantEffect under the hood, which may work differently across frameworks above. This PR normalizes it.

@filipsobol
Copy link
Member

LGTM, but as I have little knowledge of React, I'd be good if someone else looked at this PR too.

Copy link

@gorzelinski gorzelinski left a comment

Choose a reason for hiding this comment

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

I dug a little deeper. The inconsistencies may occur because of the caveats with the refs. TLDR version is this - do not write or read ref.current during rendering. If you have to read or write something during rendering, use state instead. So, the solution is probably ok. The snippet below works consistently across frameworks:

'use client'
import { useEffect, useRef } from "react";

export const Component = () => {
    const ref = useRef(null);

    console.info(ref.current);

    useEffect(() => {
        // ✅ You can read or write refs in effects
        ref.current = 'test';
    });

    return null;
};

I also added a comment that's probably unrelated.

@@ -3,7 +3,7 @@
* For licensing, see LICENSE.md.

Choose a reason for hiding this comment

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

I found a problem with our es rule - it's probably not directly related to the PR, but it's worth noting. Every file complains that the rule about the license key cannot be found: Definition for rule 'ckeditor5-rules/prevent-license-key-leak' was not found.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mati365
Copy link
Member Author

Mati365 commented Sep 30, 2024

Thanks for digging it more @gorzelinski, I'm merging then.

@Mati365 Mati365 merged commit 8886ca3 into master Sep 30, 2024
5 checks passed
@Mati365 Mati365 deleted the ck/fix-undefined-behavior branch September 30, 2024 11:04
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.

3 participants