[links] Add ability to specify domainUriPrefix instead of dynamicLinkDomain

[JYeop]

Made a change for the custom links.
If you make a link with customization(eg>abc.com/link), package throws an error.

Error: Use setDomainUriPrefix() instead, setDynamicLinkDomain() is only applicable for *.page.link and *.app.goo.gl domains.

So, I changed the v5.3.1 code just one line.
One thing to consider, setDomainUriPrefix must provided with 'https://' or 'http://'.

const link = new firebase.links.DynamicLink(
          `https://abc.com/link/?invitedby=${String(Name)},
           "abc.com/link" // Wrong
          "https://abc.com/link" // Right

Checklist

• Supports Android
• Supports iOS
• e2e tests added or updated in /tests/e2e/*
• Updated the documentation in the docs repo
  • LINK TO DOCS PR HERE
• Flow types updated
• Typescript types updated

Test Plan

Release Plan

[CATEGORY][type] [LOCATION] - Message

[CLAassistant]

All committers have signed the CLA.

[Salakar]

👋 hey @JYeop - thanks for this. This change makes sense;

One thing to consider, setDomainUriPrefix must provided with 'https://' or 'http://'

This is potentially a breaking change so would have to major version this I think; are you ok waiting for me to merge this change for v6.0.0?

[JYeop]

👋🏼 @Salakar , thanks for reply.
Absolutely 😄

[Salakar]

@JYeop awesome, thank you 🤗

And thanks for contributing 🎉

[Salakar]

@JYeop Not forgotten about this 🙃 The PR for v6 Dynamic Links is up and in Progress here: #2103

[sauravexodus]

Will this also get merge in 5.X.X?

[mikehardy]

@sauravexodus unknown but you can always use patch-package, it is made for exactly these situations and works wonderfully

[mikehardy]

Hi there! I'm helping maintain the v5 branch and I'm hoping to modify our use of semver to allow small (but important for functionality) breaking changes like this during v5's maintenance phase. Once I've had time to document that version policy change and let the messaging transmit for a little bit, I would be willing to merge this.

I just updated this PR to v5.x.x current to make sure tests still pass in preparation for that

patch-package in the meantime will be the best bet to use this

[mikehardy]

Once #2214 and invertase/react-native-firebase-docs#187 are merged and have gone through a release cycle as a v5.4.x release, I can merge this and release it as v5.5

[mikehardy]

Alright I looked into this deeply today, and while this was a functional change, it was very narrow. It hard-switched to the new domainUriPrefix with no deprecation path, and it was Android only while iOS has the same need to change API

So, I modified it and I'd like review @Salakar @JYeop

What I did was:
1- alter javascript API so link constructor can tell if you sent in an http-prefixed domain string or not. If you did, it assumes you have moved to new domainUriPrefix, if not it assumes you are still using old dynamicLinkDomain
2- alter Android to be able to use one or the other depending on which was sent
3- alter iOS to do roughly the same

There is some deprecation messaging in there as well.

What do you guys think?

There is no testing support for links in v5 so this needs careful review and testing, but it's backwards compatible now

[Salakar]

A note from me, setDomainUriPrefix on iOS SDK expects it to start with http, or at least this is what the docs example indicates; whereas android it does not, I've not yet tested the v6 implementation yet but I'm expecting some failures on it due to this.

v6 only supports setDomainUriPrefix as the old way was deprecated, be good to add a deprecation warning early in v5 but it's a bit tricky as for v6 I'm only accepting it if it starts with http still and will be stripping it off once I can confirm the iOS/Android behaviour differences.

I've tried to add as much backwards compatibility to all modules in v6 by keeping the deprecated methods, with deprecation warnings, which can be removed at a later date - so would be good to confirm which is the correct behaviour and align with v6 as much as possible.

Am running through the tests this coming week so can report back and confirm, maybe best to hold this one for a bit?

[mikehardy]

Yeah, if you are in there testing and you have questions on it, it makes sense to hold off, especially without testing support for v5.

A note from me, setDomainUriPrefix on iOS SDK expects it to start with http, or at least this is what the docs example indicates; whereas android it does not, I've not yet tested the v6 implementation yet but I'm expecting some failures on it due to this.

I think the platforms are consistent on this one. Android is documented to start with http as well and they updated their implementations even: firebase/quickstart-android@b028775 (and still there in master)

But I have not tested it myself. @JYeop indicated clearly it needed http/https though, and this was for android...

v6 only supports setDomainUriPrefix as the old way was deprecated, be good to add a deprecation warning early in v5 but it's a bit tricky as for v6 I'm only accepting it if it starts with http still and will be stripping it off once I can confirm the iOS/Android behaviour differences.

I saw that, so I detect http and if it is not http I emit deprecation warnings exactly as you'd want :-)

https://github.com/invertase/react-native-firebase/pull/2051/files#diff-c2b15a47f1d5ea1eaa13a7f57cf523c4R203

https://github.com/invertase/react-native-firebase/pull/2051/files#diff-ffbd665ef07f9c83b4b7a2183338130cR225

...well maybe not exactly. Maybe I should do those in javascript? But I agree, and want to emit warnings. I was going to note it in a related docs PR as well

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (v5.x.x@eab6862). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             v5.x.x    #2051   +/-   ##
=========================================
  Coverage          ?   72.66%           
=========================================
  Files             ?       64           
  Lines             ?     1591           
  Branches          ?        0           
=========================================
  Hits              ?     1156           
  Misses            ?      435           
  Partials          ?        0

[codecov]

Codecov Report

Merging #2051 into v5.x.x will decrease coverage by 0.1%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           v5.x.x    #2051      +/-   ##
==========================================
- Coverage   72.62%   72.53%   -0.09%     
==========================================
  Files          64       64              
  Lines        1592     1594       +2     
==========================================
  Hits         1156     1156              
- Misses        436      438       +2

[mikehardy]

I decided to go a different direction so updating the underlying SDKs was a goal of mine, and proposed #2240 which just switches completely to domainURIPrefix since dynamicLinkDomain goes away with the new SDKs.

@JYeop if that one is merged I'll close this and you'll have the feature you want, but it is important to note that I would not have known what I was doing in that part of the SDK process without the experience and code here, so this was really valuable to me

[mikehardy]

#2240 has approval - I'm going to merge it today and release v5.5.0 with it, so your proposed functionality here will be available for you. Thanks again for the work here, I appreciate it