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

feat(app-check, android): Implement app check token change listener #7309

Merged
merged 2 commits into from
Sep 4, 2023
Merged

Conversation

MrLibya
Copy link
Contributor

@MrLibya MrLibya commented Aug 22, 2023

Description

Implement app check token change listener only for android ( as the sdk don't support for iOS )

onTokenChanged function will be trigger with object contain token, expireTimeMillis, appName

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • No

Test Plan

  const onAppCheckTokenChanges = (result) => {
    console.log("onAppCheckTokenChanges: ",result)
  }

  React.useEffect(() => {
    const subscriber = appCheck().onTokenChanged(onAppCheckTokenChanges)

    return subscriber
  }, [])

@vercel
Copy link

vercel bot commented Aug 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2023 6:34pm
react-native-firebase-next ❌ Failed (Inspect) Sep 4, 2023 6:34pm

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2023

CLA assistant check
All committers have signed the CLA.

@MrLibya MrLibya changed the title + implement app check token change listener feat(app-check): Implement app check token change listener Aug 22, 2023
@MrLibya
Copy link
Contributor Author

MrLibya commented Aug 28, 2023

@mikehardy @InvertaseBot Is an anything I need todo to get reviewed ?

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Nice! The native implementation looks great - I especially like that you correctly handled the destruction of the catalyst instance ✅

At the javascript level it looks like we need a little bit of change here in order to comply with the firebase-js-sdk app-check contract, where they allow for multiple listeners for the same app, and their typings are a bit different. Here in react-native-firebase we must be a drop-in replacement for firebase-js-sdk where ever possible and I think in this case we can meet their behavior (multiple listeners) and typings,. What do you think?

packages/app-check/lib/index.d.ts Outdated Show resolved Hide resolved
packages/app-check/lib/index.js Outdated Show resolved Hide resolved
@mikehardy mikehardy added the Workflow: Waiting for User Response Blocked waiting for user response. label Aug 28, 2023
@MrLibya
Copy link
Contributor Author

MrLibya commented Aug 30, 2023

@mikehardy I've updated onTokenChanged to match params same as firebase js sdk, But onError. onCompletion isn't used, I've checked firebase class docs and it doesn't support passing error callback or throw error, Same thing if you check onAuthStateChanged for firebase auth, In js sdk it support the onError but in native android it doesn't.

@MrLibya
Copy link
Contributor Author

MrLibya commented Sep 4, 2023

@mikehardy Done 💯 I added support for multiple listeners and reset all commits into 1 to make it cleaner

@MrLibya MrLibya changed the title feat(app-check): Implement app check token change listener feat(app-check, android): Implement app check token change listener Sep 4, 2023
mikehardy
mikehardy previously approved these changes Sep 4, 2023
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks for handling the comments, and so quickly.
I think lint is going to be a bit annoyed at some of the formatting but I can auto-fix that and re-push to get this through + released, I'm doing a bunch of PRs today so it fits in my flow
Will merge as soon as CI goes green

🏆 !

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Sep 4, 2023
@mikehardy mikehardy merged commit adebe40 into invertase:main Sep 4, 2023
13 of 16 checks passed
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.

3 participants