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.

Sign up for GitHub

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

Include memory stats when memory warning occurs. #4891

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Include memory stats when memory warning occurs. #4891

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,20 @@ import NotificationBanner from './components/NotificationBanner'
import Connectivity from './components/Connectivity'
import ErrorHandler from './components/common/ErrorHandler'
import {bugsnagNotify} from 'src/utils/bugsnagConfig'
import DeviceInfo from 'react-native-device-info'

const App = () => {
const [store, setStore] = useState<any>(null)

useEffect(() => {
// This seems to be undocumented event.
AppState.addEventListener('memoryWarning', () => {
const extras = {currentState: AppState.currentState}
AppState.addEventListener('memoryWarning', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're doing this on a top-level component we probably don't need to worry about removing this listener later, but just something to think about if we start to see performance drop-off

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I guess this listener has been in for a while already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I implemented the listener (it was a long time ago), it was pre-useEffect and AppState.addEventListener was supposed to happen only once per app invocation. Hence, I didn't see a need to remove it.

Are you implying that, after it was moved into useEffect, it can happen more than once?

If so, then ... how about moving it back outside of useEffect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Naw, it's fine as is

const extras = {
currentState: AppState.currentState,
maxMemory: await DeviceInfo.getMaxMemory(),
totalMemory: await DeviceInfo.getTotalMemory(),
usedMemory: await DeviceInfo.getUsedMemory(),
}
bugsnagNotify(new Error('AppState.memoryWarning fired'), 'memory_warning', extras)
analytics.track('memory_warning', extras)
})
Expand Down