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 support for utmParameters #5593

Merged
merged 5 commits into from
Aug 15, 2021

Conversation

Minishlink
Copy link
Contributor

Description

This adds the information about utm parameters that was setup in the dynamic link. This can be used to then store that information elsewhere or react to it somehow.

Related issues

Might be useful for #4554

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:

🔥

@vercel
Copy link

vercel bot commented Aug 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/6ujoieM1h3PYrvDv1nrDZ3WaQZgx
✅ Preview: Canceled

[Deployment for 15daa15 canceled]

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/2SMWZdH3fYGikgxXYjvjaYQqzwbp
✅ Preview: https://react-native-firebase-git-fork-minishlink-utmp-c98196-invertase.vercel.app

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #5593 (2053796) into master (20158ce) will increase coverage by 2.99%.
The diff coverage is n/a.

❗ Current head 2053796 differs from pull request most recent head 15daa15. Consider uploading reports for the commit 15daa15 to get more accurate results

@@            Coverage Diff             @@
##           master    #5593      +/-   ##
==========================================
+ Coverage   71.11%   74.10%   +2.99%     
==========================================
  Files         107      107              
  Lines        4420     4420              
  Branches      942      942              
==========================================
+ Hits         3143     3275     +132     
+ Misses       1179     1073     -106     
+ Partials       98       72      -26     

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.

Oh this is really cool! Thank you for posting this, it should be really useful.
firebase-js-sdk does not have a links module to compare types with and it seems to me Record<string, string> is as good as any

My only hesitation - especially since I think it's an easy addition? - is that it could be verified much more strongly in the e2e test

I scanned the test carefully and left two specific notes for the initial / onlink test, and then for the resolve link if you added the params at the top of this one (where minimum version was added) and verified it at the bottom (where minimum version was checked) I think all the work here would be fully verified

https://github.com/invertase/react-native-firebase/pull/5593/files#diff-b885fc4a786a779f37b59dd276f88b9625d573aee097b53e5d8b4f47789a306aR80-R101

The code itself in Java/Objective-C looks great but I remember when I added resolveLink my code also looked great to me then I added verification in e2e and of course there was some subtle problem 😆

If you could try that and see if it really verifies in the e2e test I'd appreciate it and could merge+release straight away

@@ -155,6 +155,7 @@ describe('dynamicLinks()', function () {

dynamicLink.should.be.an.Object();
dynamicLink.url.should.equal('https://rnfirebase.io');
dynamicLink.utmParameters.should.eql({});
Copy link
Collaborator

Choose a reason for hiding this comment

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

With a quick addition of utm params to TEST_LINK the population may be verified I think?

@@ -172,6 +173,7 @@ describe('dynamicLinks()', function () {

spy.getCall(0).args[0].should.be.an.Object();
spy.getCall(0).args[0].url.should.equal('https://invertase.io/hire-us');
spy.getCall(0).args[0].utmParameters.should.eql({});
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here - quick addition of utm params to TEST_LINK2 and this may be verified

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar Workflow: Waiting for User Response Blocked waiting for user response. labels Aug 13, 2021
@mikehardy
Copy link
Collaborator

There weren't a lot of PRs in flight in the repo so I chose this moment to integrate an auto formatter (#5596) - which necessarily cause all sorts of conflicts in this PR. That was no fault of the PR's of course, so I merged it and I believe I have correctly resolved all the conflicts with the last commit here if you pull it locally

…base/dynamiclinks/ReactNativeFirebaseDynamicLinksModule.java
…base/dynamiclinks/ReactNativeFirebaseDynamicLinksModule.java
@Minishlink
Copy link
Contributor Author

Minishlink commented Aug 14, 2021

Unfortunately it is not possible to test these cases (or am I missing something?), as apparently Firebase does not rely on the long link and does not parse utm parameters on a short link, it only works with a short link that has utm parameters inside. This should be more understandable:

  • https://reactnativefirebase.page.link/?link=https://invertase.io/hire-us&apn=com.invertase.testing&utm_source=github : you won't get any utm parameters from getUtmParameters
  • https://reactnativefirebase.page.link/shortLinkWithoutUtmParameters?utm_source=github : you won't get any utm parameters from getUtmParameters
  • https://reactnativefirebase.page.link/shortLinkWithUtmParameters : you will get the utm parameters. These utm parameters corresponds to the utm parameters you put when generating that short link in the Firebase dashboard (or through the in app builder I guess)

I think the reasoning behind this is that from the first two links you can always infer the utm parameters by parsing the url (that you can get from the deeplink rather than the dynamic link)

@mikehardy
Copy link
Collaborator

OK I see what you mean. However, it's possible to generate a short link with utm params, then resolve it isn't it? That's what I meant to suggest. Worst case I have access to the dashboard for the testing project and can make a link with utm params in it, I was just hoping it was testable with completely self contained data fixture

@mikehardy
Copy link
Collaborator

I had some time to check this while waiting for some other tests to run, if you use buildShortLink in the e2e test then the params do come through - I've got it locally, I'll push to your branch then merge this if I can, or if I mess up the git magic somehow I'll carry the commit in a fresh PR. Either way, all of your native parsing and everything is 💯 - it all checked out first time when I ran it here locally

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.

the changes needed to do the test with UTM params in it exposed a couple other problems, and fixing those on this branch + in this PR isn't good practice. This appears to be working well in local testing so I'm going to merge it then dig in to the other issues in the area.

Thanks again for posting this!

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.

2 participants