fix(jumpstart): generate link outside of firebase sdk #5149
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5149 +/- ##
=======================================
Coverage 85.79% 85.80%
=======================================
Files 742 742
Lines 30097 30101 +4
Branches 5188 5188
=======================================
+ Hits 25822 25828 +6
+ Misses 4039 4037 -2
Partials 236 236
Continue to review full report in Codecov by Sentry.
|
| link: WEB_LINK, | ||
| }) | ||
| ) | ||
| const dynamicUrl = new URL(dynamicLink) |
There was a problem hiding this comment.
maybe i'm misunderstanding something here, but do we need to wrap this with another new URL constructor? isn't this essentially new URL(new URL(stuff...))?
There was a problem hiding this comment.
oops, i meant to remove that. thanks for the catch!
| // the firebase dynamic links sdk encodes dots and dashes even though it is | ||
| // not strictly required for urls. calling searchParams.set seems to transform | ||
| // __all__ search params to be url encoded, where dots and dashes are no | ||
| // longer encoded. This is probably okay, but to be extra safe we will put | ||
| // back the encoding ourselves. | ||
| const searchParams = dynamicUrl.search.replace(/\./g, '%2E').replace(/-/g, '%2D') | ||
| return `${dynamicUrl.origin}/${searchParams}` |
There was a problem hiding this comment.
Could we "not" be extra safe and remove this?
Feels a bit surprising we're only doing it for dots and dashes.
There was a problem hiding this comment.
i thought about this and tried consuming a link without the encoding on a simulator and it does work, but since the firebase sdk were producing links that are encoded in this way i thought it was safest to keep it incase somewhere in the sdk when consuming deep links on some specific devices, the encoding becomes important :/ since i don't understand why they did the encoding in the first place, i was thinking it wasn't worth the risk of removing it...at least this mess is well documented and inside this function?
There was a problem hiding this comment.
oh the dots and dashes are hard coded because we know what the deep link will look like :)
### Description As the title. ### Test plan Tested that the generated link is exactly the same as before this change. ### Related issues n/a ### Backwards compatibility n/a ### Network scalability n/a
Description
As the title.
Test plan
Tested that the generated link is exactly the same as before this change.
Related issues
n/a
Backwards compatibility
n/a
Network scalability
n/a