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

Memory Leak : HybridProvider #5547

Open
2 of 5 tasks
MarkLester10 opened this issue Nov 11, 2022 · 9 comments
Open
2 of 5 tasks

Memory Leak : HybridProvider #5547

MarkLester10 opened this issue Nov 11, 2022 · 9 comments

Comments

@MarkLester10
Copy link

MarkLester10 commented Nov 11, 2022

Description

When you close the app there is a memory leak in useEffect HybridProvider:

CodeSandbox/Snack link

Private Repo

Steps to reproduce

  1. Open the app
  2. Close
  3. Error in Metro appears
ERROR Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
in HybridProvider (at NativeBaseProvider.tsx:90)

NativeBase Version

3.4.5

Platform

  • Android
  • CRA
  • Expo
  • iOS
  • Next

Other Platform

No response

Additional Information

Project is using RN 0.68.2

@surajahmed
Copy link
Collaborator

@MarkLester10 Thanks for reporting.
We'll look into this. Hope to fix this in the coming release.

@kevindice
Copy link
Contributor

Pretty sure I'm seeing the same issue but with a slightly different message:

    console.error
      Warning: Internal React error: Attempted to capture a commit phase error inside a detached tree. This indicates a bug in React. Likely causes include deleting the same fiber more than once, committing an already-finished tree, or an inconsistent return pointer.
      
      Error message:
      
      TypeError: Cannot read properties of undefined (reading 'remove')
          at children (/Users/kdice/company-redacted/mobile-apps/node_modules/native-base/lib/commonjs/core/hybrid-overlay/HybridProvider.tsx:9:3)
          at disableCSSMediaQueries (/Users/kdice/company-redacted/mobile-apps/node_modules/native-base/lib/commonjs/utils/useResponsiveQuery/ResponsiveQueryProvider.tsx:20:12)
          at children (/Users/kdice/company-redacted/mobile-apps/node_modules/react-native-safe-area-context/jest/mock.js:26:24)
          at children (/Users/kdice/company-redacted/mobile-apps/node_modules/native-base/lib/commonjs/utils/createContext.tsx:9:13)
          at colorModeManager (/Users/kdice/company-redacted/mobile-apps/node_modules/native-base/lib/commonjs/core/NativeBaseProvider.tsx:52:5)

Fills up the Jest output quite a bit.

@reactionic27
Copy link

@surajahmed Is there any updates about this issue? I am having same issue in Jest.
My issue is same with @kevindice

@bojackhorseman0309
Copy link

I'm having the same issue like @kevindice when running my tests after updating to Expo SDK 46, also using latest Native Base.

Is this already fixed? Did someone found anything?

@kevindice
Copy link
Contributor

kevindice commented Mar 19, 2023

My team has been using a yarn patch on it so far:

Screenshot_20230319-140112.png

(apologies for the screenshot, am on mobile)

edit:

Filename: /.yarn/patches/native-base-npm-3.4.28-a8ecceae4d.patch

diff --git a/lib/commonjs/core/color-mode/hooks.js b/lib/commonjs/core/color-mode/hooks.js 
 index 76751c096376cd2097b84b220fc11bb82be1b53c..ebe26ce20a99c97790aca157fcae80c1c7e5496e 100644 
 --- a/lib/commonjs/core/color-mode/hooks.js 
 +++ b/lib/commonjs/core/color-mode/hooks.js 
 @@ -56,7 +56,7 @@ const useAppState = () => { 
          } else { 
            // React Native >= 0.65 
            // @ts-ignore:next-line ignoring ts error as devDependency contains "@types/react-native" < 0.65 
 -          subsription.remove(); 
 +          subsription?.remove(); 
          } 
        }; 
      }

@bojackhorseman0309
Copy link

@kevindice Ohhhh yeah indeed, just did a quick change on the file and it indeed doesn't show anymore. Thanks for the quick response.

I saw that you reviewed this PR that introduce that code #5359

If the fix is to use the optional chaining, why it hasn't been done? Is there a more deeper issue with this? Or is it that it just haven't been done?

@kevindice
Copy link
Contributor

@kevindice Ohhhh yeah indeed, just did a quick change on the file and it indeed doesn't show anymore. Thanks for the quick response.

I saw that you reviewed this PR that introduce that code #5359

If the fix is to use the optional chaining, why it hasn't been done? Is there a more deeper issue with this? Or is it that it just haven't been done?

Honestly not sure

@r3dm1ke
Copy link

r3dm1ke commented Apr 26, 2023

Is there an update for this one? Can I open a PR that adds optional chaining?

@bojackhorseman0309
Copy link

bojackhorseman0309 commented Apr 26, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants