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(dynamic-links): add expo config plugin for dynamic-links #6650

Merged
merged 9 commits into from
Nov 2, 2022

Conversation

austin43
Copy link
Contributor

@austin43 austin43 commented Nov 1, 2022

Description

This pull request is intended to add an expo config plugin for @react-native-firebase/dynamic-links which resolves a possible race condition with dynamicLinks().getInitialLink(). The mechanism for this fix is described in this comment.

Related issues

#2660
#4548
#4548 (comment)

Release Summary

feat(dynamic-links): add expo config plugin for dynamic-links
docs(dynamic-links): add note for iOS swizzling workaround

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

This PR codifies testing that has been done related to this comment and merely modifies the AppDelegate.

To test, add the @react-native-firebase/dynamic-links plugin to the list of RNFB plugins, and verify the AppDelegate has been modified.

Functionally, real world dynamic link matching is inherently hard to test due to the end-to-end nature of the app install process. My best recommendation for testing here is

  • generate a dynamic link that points to your app that is live on the App Store
  • return to the Home Screen
  • install a dev version of your app which contains the fix
  • verify getInitialLink is called and contains your expected payload

@vercel
Copy link

vercel bot commented Nov 1, 2022

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

Name Status Preview Comments Updated
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Nov 2, 2022 at 0:47AM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Nov 2, 2022 at 0:47AM (UTC)

@CLAassistant
Copy link

CLAassistant commented Nov 1, 2022

CLA assistant check
All committers have signed the CLA.

mikehardy
mikehardy previously approved these changes Nov 2, 2022
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.

Thank you! As I carefully mention to everyone - just to set expectations - I don't use Expo personally so I cannot say if this is perfect or not, but I am more than happy to facilitate community support by merging and releasing quickly. And it looks good on a scan, so - let's go :-). We can always handle issues with followups.

This should help quite a few people

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Nov 2, 2022
@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Merging #6650 (882fdc6) into main (d654d00) will increase coverage by 0.09%.
The diff coverage is 79.42%.

@@             Coverage Diff              @@
##               main    #6650      +/-   ##
============================================
+ Coverage     53.98%   54.06%   +0.09%     
  Complexity      690      690              
============================================
  Files           215      216       +1     
  Lines         10738    10772      +34     
  Branches       1689     1696       +7     
============================================
+ Hits           5796     5823      +27     
- Misses         4650     4657       +7     
  Partials        292      292              

@mikehardy
Copy link
Collaborator

spelling:

53:115-53:121 warning Github is misspelt github retext-spell

It is in the spelling dictionary as github all lower, probably just re-use that so no spelling dictionary mod is needed

jest:

packages/dynamic-links/plugin/__tests__/iosPlugin.test.ts:8:1 - error TS2582: Cannot find name 'describe'. Do you need to install type definitions for a test runner? Try `npm i --save-dev @types/jest` or `npm i --save-dev @types/mocha`.

    8 describe('Config Plugin iOS Tests', function () {
      ~~~~~~~~
    packages/dynamic-links/plugin/__tests__/iosPlugin.test.ts:9:3 - error TS2304: Cannot find name 'beforeEach'.

    9   beforeEach(function () {
        ~~~~~~~~~~
    packages/dynamic-links/plugin/__tests__/iosPlugin.test.ts:10:5 - error TS2304: Cannot find name 'jest'.

    10     jest.resetAllMocks();
           ~~~~
    packages/dynamic-links/plugin/__tests__/iosPlugin.test.ts:13:3 - error TS25[82](https://github.com/invertase/react-native-firebase/actions/runs/3373357634/jobs/5600473375#step:7:83): Cannot find name 'it'. Do you need to install type definitions for a test runner? Try `npm i --save-dev @types/jest` or `npm i --save-dev @types/mocha`.

    13   it('tests changes made to old AppDelegate.m (SDK 42)', async function () {
         ~~
    packages/dynamic-links/plugin/__tests__/iosPlugin.test.ts:18:5 - error TS2304: Cannot find name 'expect'.

    18     expect(result).toMatchSnapshot();
           ~~~~~~

These global symbols need to be imported now that we are using the built-in jest typescript types

iOS failure:


  1) multi-factor
       "before each" hook for "can not enroll with phone authentication (unsupported primary factor)":
     NativeFirebaseError: [auth/email-already-in-use] The email address is already in use by another account.
      at FirebaseAuthModule.createUserWithEmailAndPassword (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=io.invertase.testing&sourceMapURL=true:141140:28)
      at Context.<anonymous> (/Users/runner/work/react-native-firebase/react-native-firebase/packages/auth/e2e/multiFactor.e2e.js:16:27)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

🤔 that looks unrelated though it was an area of recent work. Assume it will clear when the above fixes trigger a re-run of CI

@mikehardy
Copy link
Collaborator

Took a little work but got it shaped up! Let's go

@mikehardy mikehardy enabled auto-merge (squash) November 2, 2022 14:54
@mikehardy mikehardy disabled auto-merge November 2, 2022 14:54
@mikehardy mikehardy merged commit e558ad7 into invertase:main Nov 2, 2022
@maxwowo
Copy link

maxwowo commented Nov 3, 2022

Just tried to use the @react-native-firebase/[email protected] package, looks like ./plugin/build isn't being generated. Both @react-native-firebase/perf and @react-native-firebase/crashlytics have this directory but it's missing for @react-native-firebase/dynamic-links

image

image

@mikehardy
Copy link
Collaborator

If there is something missing from the PR, perhaps in the package.json of the package to trigger the build, please send a PR - I won't have time to investigate for a while and Expo support is best provided by people actually using it (read as: not me, I don't use Expo so am not good at supporting it at the moment, sorry)

@austin43
Copy link
Contributor Author

austin43 commented Nov 3, 2022

@maxwowo I missed the build commands in package.json, and am submitting a new PR right now.

@mikehardy
Copy link
Collaborator

releasing the fix from @austin43 now (thank you!) - v16.4.2 when its up in a few moments

@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Nov 5, 2022
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