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(messaging): onNotificationOpenedApp callable return type #3641

Merged
merged 1 commit into from
May 12, 2020

Conversation

Gyran
Copy link
Contributor

@Gyran Gyran commented May 12, 2020

messaging().onNotificationOpenedApp returns a function that can be used for unsubscribing from this listener
currently it's typed as returning void and you will get type errors if you try to call the returned function.

🔥

Description

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

`messaging().onNotificationOpenedApp` returns a function that can be used for unsubscribing from this listener
currently it's typed as returning `void` and you will get type errors if you try to call the returned function.

🔥
@CLAassistant
Copy link

CLAassistant commented May 12, 2020

CLA assistant check
All committers have signed the CLA.

@mikehardy
Copy link
Collaborator

I'm actually not sure why this method is even in the package. RNFBv6 isn't going to handle this stuff at all I don't think? No notifications in RNFBv6 @Salakar

@Gyran
Copy link
Contributor Author

Gyran commented May 12, 2020

It's being called in my app when I click the notification

@Salakar
Copy link
Member

Salakar commented May 12, 2020

I'm actually not sure why this method is even in the package. RNFBv6 isn't going to handle this stuff at all I don't think? No notifications in RNFBv6 @Salakar

Is a fine line on what to include and not include 😅, this allows you to at least know when a Remote Notification (FCM) causes your app to open by the user pressing it - which is something we decided you need to have really for FCM to be usable for remote notifications

--

As for the change it LGTM, thanks @Gyran

@Salakar Salakar changed the title fix(messaging) onNotificationOpenedApp callable return type fix(messaging): onNotificationOpenedApp callable return type May 12, 2020
@Salakar Salakar merged commit cd5cb23 into invertase:master May 12, 2020
stefkampen pushed a commit to stefkampen/react-native-firebase that referenced this pull request Jun 6, 2020
hmhm2292 pushed a commit to hmhm2292/react-native-firebase that referenced this pull request Jul 13, 2021
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.

4 participants