[Android] AppState by skv-headless · Pull Request #5152 · react/react-native

skv-headless · 2016-01-06T13:33:25Z

No description provided.

facebook-github-bot · 2016-01-06T13:34:29Z

By analyzing the blame information on this pull request, we identified @vjeux, @zjj010104 and @mkonicek to be potential reviewers.

mkonicek · 2016-01-06T13:45:52Z

We have this module internally and want to open source it, let me compare the implementations.

mkonicek · 2016-01-06T13:46:10Z

Thanks for working on this!

skv-headless · 2016-01-06T13:51:27Z

No problem, thanks for quick response. If I can help you somehow to deliver it faster just let me know.

mkonicek · 2016-01-12T14:19:44Z

I think we should still keep AppStateIOS.ios.js, AppStateIOS.android.js but introduce a new API AppState.js, similar to what we did with Switch and Alert for example.

Here's the internal module:
Java: https://gist.github.com/mkonicek/039f35748c0a073dd1b5
JS: https://gist.github.com/mkonicek/00c3638a4cf5d9ed8db7

If you can make AppState module that has a single API on Android and iOS that would be amazing. Looks like the AppStateAndroid.js might be redundant? Unfortunately the iOS and Android version were implemented by two different people but we've learned from that and now always think about both platforms at once when adding new APIs.

mkonicek · 2016-01-12T14:19:56Z

Sorry for the delay on this!

skv-headless · 2016-01-13T07:53:15Z

I'll update pr in few days. Could you please don't release AppStateAndroid? I want to make it same for both platforms as you said.

facebook-github-bot · 2016-01-15T14:47:16Z

@skv-headless updated the pull request.

eslint-bot · 2016-01-15T14:57:19Z

property AppState Property not found in Object.create

You can ignore this lint warning.

facebook-github-bot · 2016-01-15T14:59:17Z

@skv-headless updated the pull request.

skv-headless · 2016-01-15T15:04:02Z

@mkonicek could you please review?

eslint-bot · 2016-01-15T15:07:51Z

property AppState Property not found in Object.create

satya164 · 2016-01-15T15:29:22Z

Since we're introducing a new API, we should use promises instead of callbacks. Refer #4971

satya164 · 2016-01-17T12:12:55Z

Maybe also have a method sendAppStateEvent?
privatevoidsendAppStateEvent(WritableMapappState) { getReactApplicationContext().getJSModule(RCTDeviceEventEmitter.class) .emit("appStateDidChange", appState); }

mkonicek · 2016-01-18T21:40:00Z

Thanks again for working on it! I'm a bit swamped but will try to review tomorrow.

facebook-github-bot · 2016-01-19T08:21:15Z

@skv-headless updated the pull request.

eslint-bot · 2016-01-19T08:28:48Z

property AppState Property not found in Object.create

mkonicek · 2016-01-19T18:21:29Z

Is AppState.js a copy of AppStateIOS.ios.js or have you made any changes? Knowing that would make the review easier.

mkonicek · 2016-01-19T18:26:40Z

Default state should be "uninitialized" as per the internal module: https://gist.github.com/mkonicek/039f35748c0a073dd1b5
What's the rationale for setting it to "active"?

mkonicek · 2016-01-19T18:27:06Z

nit: Please use constants like in https://gist.github.com/mkonicek/039f35748c0a073dd1b5

mkonicek · 2016-01-19T18:36:29Z

Getting close! Very excited!

skv-headless · 2016-01-19T18:36:33Z

AppState.js is a copy of AppStateIOS.ios.js

mkonicek · 2016-01-19T18:38:27Z

AppState.js is a copy of AppStateIOS.ios.js

Thanks! I guessed that was the case.

skv-headless · 2016-01-19T18:40:19Z

@mkonicek agree with all notes, thank you for review. I'll try to fix tomorrow.

satya164 · 2016-01-19T21:09:07Z

AppState.js is a copy of AppStateIOS.ios.js

If that's the case, can you also get rid of AppState.ios.js, and for platform specific things, add a Platform.OS check?

skv-headless · 2016-01-20T07:03:03Z

@satya164 there is no platform specific code. At first version of PR I deleted AppStateIOS.js module. But @mkonicek asked me to left this component as it is. I think for compatibility with previous version. I copied it to keep them AppState and AppStateIOS independent.

satya164 · 2016-01-20T07:31:32Z

If the APIs are exactly the same, maybe we can just require the AppState file within AppStateIOS and re-export?

mkonicek · 2016-01-20T18:02:15Z

I like the idea of making AppStateIOS a wrapper around AppState. We can add deprecation warnings and finally delete AppStateIOS a few releases down the line. Let's do that in a separate PR though, this one is already large enough.

mkonicek · 2016-01-20T18:10:01Z

@skv-headless Cool! Once you address the comments I think we can merge this.

facebook-github-bot · 2016-01-20T19:52:16Z

@skv-headless updated the pull request.

skv-headless · 2016-01-20T20:08:44Z

@mkonicek updated.
Plan for the future:

deprecation warning to AppStateIOS
android memory event

eslint-bot · 2016-01-20T20:52:00Z

property AppState Property not found in Object.create

mkonicek · 2016-01-21T16:42:32Z

Awesome!

@facebook-github-bot shipit

facebook-github-bot · 2016-01-21T16:43:32Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/581762778640408/int_phab to review.

Summary: Closes#5152 Reviewed By: svcscm Differential Revision: D2850250 Pulled By: mkonicek fb-gh-sync-id: 0b5063fa7121d4e304a70da8573c9ba1d05a757c

jacygogogo · 2020-01-16T09:47:15Z

guy can you tell me what is the difference between AppStateIOS and AppState.

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 6, 2016

skv-headless force-pushed the android-app-state branch from 7820338 to bf49bdeCompare January 15, 2016 14:46

eslint-bot reviewed Jan 15, 2016
View reviewed changes

skv-headless force-pushed the android-app-state branch from bf49bde to 5d3a887Compare January 15, 2016 14:59

eslint-bot reviewed Jan 15, 2016
View reviewed changes

Comment threadExamples/UIExplorer/AppStateExample.js
eslint-botJan 15, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property AppState Property not found in Object.create

satya164 reviewed Jan 17, 2016
View reviewed changes

skv-headless force-pushed the android-app-state branch from 5d3a887 to 1d894aeCompare January 19, 2016 08:20

eslint-bot reviewed Jan 19, 2016
View reviewed changes

Comment threadExamples/UIExplorer/AppStateExample.js
eslint-botJan 19, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property AppState Property not found in Object.create

mkonicek reviewed Jan 19, 2016
View reviewed changes

mkonicek added the GH Review: needs-revision label Jan 19, 2016

[Android] AppState
617a1b3

skv-headless force-pushed the android-app-state branch from 1d894ae to 617a1b3Compare January 20, 2016 19:51

eslint-bot reviewed Jan 20, 2016
View reviewed changes

Comment threadExamples/UIExplorer/AppStateExample.js
eslint-botJan 20, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property AppState Property not found in Object.create

bestander mentioned this pull request Jan 21, 2016
[release-0.19] Commits to cherry pick #5462
Closed
10 tasks

ghost closed this in c2d75d7Jan 21, 2016

mkonicek pushed a commit that referenced this pull request Jan 29, 2016
Android AppState
90dd337
Summary: Closes#5152 Reviewed By: svcscm Differential Revision: D2850250 Pulled By: mkonicek fb-gh-sync-id: 0b5063fa7121d4e304a70da8573c9ba1d05a757c

This pull request was closed.

Uh oh!

Conversation

skv-headless commented Jan 6, 2016

Uh oh!

facebook-github-bot commented Jan 6, 2016

Uh oh!

mkonicek commented Jan 6, 2016

Uh oh!

mkonicek commented Jan 6, 2016

Uh oh!

skv-headless commented Jan 6, 2016

Uh oh!

mkonicek commented Jan 12, 2016

Uh oh!

mkonicek commented Jan 12, 2016

Uh oh!

skv-headless commented Jan 13, 2016

Uh oh!

facebook-github-bot commented Jan 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 15, 2016

Uh oh!

skv-headless commented Jan 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satya164 commented Jan 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Jan 18, 2016

Uh oh!

facebook-github-bot commented Jan 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Jan 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Jan 19, 2016

Uh oh!

skv-headless commented Jan 19, 2016

Uh oh!

mkonicek commented Jan 19, 2016

Uh oh!

skv-headless commented Jan 19, 2016

Uh oh!

satya164 commented Jan 19, 2016

Uh oh!

skv-headless commented Jan 20, 2016

Uh oh!

satya164 commented Jan 20, 2016

Uh oh!

mkonicek commented Jan 20, 2016

Uh oh!

mkonicek commented Jan 20, 2016

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

skv-headless commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Jan 21, 2016

Uh oh!

facebook-github-bot commented Jan 21, 2016

Uh oh!

jacygogogo commented Jan 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects