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 pop initial for firebase #807

Merged
merged 1 commit into from
Aug 1, 2018
Merged

Conversation

steelbrain
Copy link

A cleaned up version of #793

@Gp2mv3
Copy link
Contributor

Gp2mv3 commented Jul 31, 2018

Hey, did you check it with the example app in the example folder of the repo ?

@Gp2mv3 Gp2mv3 added the To test This issue or PR has to be tested label Jul 31, 2018
@akbokha
Copy link

akbokha commented Jul 31, 2018

I will close my PR (#793) if this one is tested and merged (would save me some time :)

@steelbrain
Copy link
Author

@Gp2mv3 For some reason the example app doesn't run for me, not even in master so if someone else could please try, that would be great.

@akbokha Thank for the other PR btw! It was the base for building this one

@Gp2mv3
Copy link
Contributor

Gp2mv3 commented Aug 1, 2018

Hey,
Thanks for the test. Are you sure you are testing the right app ?
Because RNBranch is a completely different package (branch.io) and it isn't included in the example app.

Check that you don't have two react-native bundlers running and that you are in the right folder.

Regards,

Gp2mv3

@steelbrain
Copy link
Author

Apologies, I had another bundler running. Good catch!

The test app works, I've tested the new changes with in different scenarios. Here are the results from my testing:

  • When a previously closed app is launched, a new JS context is created and initial pop works
  • When a notification is clicked from when the app was minimized, JS execution is resumed (JS is not restarted) so it just triggers onNotification like expected
  • When the app is running and in foreground, onNotification is triggered as expected

I did notice that sometimes people override onNewIntent in their MainApplication in their RN projects, and in that new overriden function they do something like setIntent(newIntent), which does NOT trigger RN new intent handlers and therefore breaks this module.

To the people I've who do what I've mentioned above, they should instead use super.onNewIntent(newIntent) instead and then write their custom logic below that so RN can properly fire callbacks. I think this is what is going on in #793 (comment)

@Gp2mv3 Gp2mv3 merged commit b61ce08 into zo0r:master Aug 1, 2018
@Gp2mv3
Copy link
Contributor

Gp2mv3 commented Aug 1, 2018

Don't worry, I already did the mistake a lot of time. 😉

If everything goes well, we can merge !

@akbokha
Copy link

akbokha commented Aug 2, 2018

Nice work guys! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
To test This issue or PR has to be tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants