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

refactor: Remove the unnecessary logic #1928

Merged
merged 1 commit into from Oct 27, 2022

Conversation

Flcwl
Copy link
Contributor

@Flcwl Flcwl commented Oct 19, 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

#1927

💡 Background and solution

const useUnmountedRef = () => {
  const unmountedRef = useRef(false);
  useEffect(() => {
-    unmountedRef.current = false;
    return () => {
      unmountedRef.current = true;
    };
  }, []);
  return unmountedRef;
};

📝 Changelog

Language Changelog
🇺🇸 English Remove the unnecessary logic in unmountedRef hook to optimize performance
🇨🇳 Chinese 移除 unmountedRef 中非必要的逻辑,以优化性能

☑️ 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

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hchlq
Copy link
Collaborator

hchlq commented Oct 19, 2022

这个逻辑确实没啥用...

@hchlq
Copy link
Collaborator

hchlq commented Oct 19, 2022

辛苦帮忙补个单侧...

@Flcwl
Copy link
Contributor Author

Flcwl commented Oct 21, 2022

辛苦帮忙补个单侧...

How can I do for it? This change should not need to add additional tests?

@hchlq
Copy link
Collaborator

hchlq commented Oct 21, 2022

The value of the ref should be false after the component is unmounted and then remounted?

@Flcwl
Copy link
Contributor Author

Flcwl commented Oct 21, 2022

The value of the ref should be false after the component is unmounted and then remounted?

@hchlq Hi, My friend~~ I think don't need this case? because the React will reinit react hook state when the component is mounted.
And I also don't see the api like remount in the dependency @testing-library/react-hooks, I don't know how to do it.

@hchlq
Copy link
Collaborator

hchlq commented Oct 21, 2022

Get it. If not, it's not necessary.

@crazylxr
Copy link
Collaborator

LGTM

@miracles1919
Copy link
Collaborator

Here is a problem in react 18 StrictMode, effects will run twice in development, see beta website

// react 18 StrictMode

// effect created
unmountedRef => false

// effect destoryed
unmountedRef => true

// effect created
unmountedRef => true

you can see, it's necessary

miracles1919 added a commit to miracles1919/hooks that referenced this pull request Oct 31, 2022
crazylxr pushed a commit that referenced this pull request Oct 31, 2022
@Flcwl
Copy link
Contributor Author

Flcwl commented Oct 31, 2022

Here is a problem in react 18 StrictMode, effects will run twice in development, see beta website

// react 18 StrictMode

// effect created
unmountedRef => false

// effect destoryed
unmountedRef => true

// effect created
unmountedRef => true

you can see, it's necessary

Oh, So Sorry~
@miracles1919 Thank you very much for your timely feedback, I also found problems in React18 indeed, wrong happened when saving changed files: https://codesandbox.io/s/stoic-cdn-nzjx96?file=/src/App.js

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.

None yet

5 participants