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(useWebSocket): add init phase to pass React 18 Strict Mode #1923

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KONY128
Copy link

@KONY128 KONY128 commented Oct 17, 2022

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

Related problems, root cause analysis and description of this fix is in #1922

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English Fix the problem that useWebSocket is not available in React 18 Strict Mode
🇨🇳 Chinese 修复了 useWebSocket 在 React 18 Strict Mode 下不可用的问题

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2022

CLA assistant check
All committers have signed the CLA.

@miracles1919
Copy link
Collaborator

Thanks for your contribution ~

@KONY128
Copy link
Author

KONY128 commented Oct 31, 2022

I appreciate your review. A patch has been committed to my source code for your comments.

The initialization of the unmountedRef is handled in the hook useUnmountedRef at the mount phase after the intentional unmount. So it's ok in the React 18 Strict Mode.

// The following is the source code of the hook `useUnmountedRef`
import { useEffect, useRef } from 'react';

const useUnmountedRef = () => {
  const unmountedRef = useRef(false);
  useEffect(() => {
    unmountedRef.current = false; // Here, the unmountedRef is set to false at every mount process
    return () => {
      unmountedRef.current = true;
    };
  }, []);
  return unmountedRef;
};

export default useUnmountedRef;

But I decided to edit my code after all. It's because, in my origin code, the initialization of the internal states is separated into different places, which could be confusing.

PTAL. Thx.

@KONY128 KONY128 requested a review from miracles1919 October 31, 2022 03:51
@miracles1919
Copy link
Collaborator

The initialization of the unmountedRef is handled in the hook useUnmountedRef at the mount phase after the intentional unmount. So it's ok in the React 18 Strict Mode.

I see, the code I see is being merged, #1928

unmountedRef.current = false has been removed(this is problematic and I will reset)... So your origin code is right!

Copy link
Collaborator

@miracles1919 miracles1919 left a comment

Choose a reason for hiding this comment

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

A problem I found while I'm writing test cases is that onError is triggered

// demo 

const Demo = () => {
  // ...
    useWebSocket(
    'wss://demo.piesocket.com/v3/channel_1?api_key=VCXCEuvhGcBDP7XhiJJUDvR1e1D3eiVjgZ9VRiaV&notify_self',
    {
+      onError: (err) => console.log('onError', err),
    },
  );
}

export default () => {
+  <StrictMode>
    <Demo />
+  </StrictMode>;
};

image

@hchlq
Copy link
Collaborator

hchlq commented Mar 22, 2023

@KONY128 Are you still following up on this PR?

@nextdooroldwang
Copy link

v3.7.4 遇到同样问题

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.

6 participants