Start using proper URL objects instead of string concatenation

[chrisbobbe]

EDIT: Blocked on #3781, the RN 0.61 upgrade, unfortunately; see my comments here, up until #4081 (comment), below. Looks like this is unblocked; see #4081 (comment) for a way to avoid depending on a file newly introduced in RN v0.61.

---

Arising especially from #3819 (comment), #3819 (comment), and #3567 (comment), which all deal with something going wrong with constructing URLs, here's an issue for using something more reliable and consistent than string concatenation and various regexes.

Issues like #4058 may benefit from this too?

#4082 is a workaround that has our first TODO pointing to the eventual resolution of this issue.

It looks like React Native has a polyfill for the URL Web API, but it's been cut down to reduce JS bundle size. https://github.com/charpeni/react-native-url-polyfill#why-do-we-need-this is a more complete polyfill (it says it would add 73.41 KB to the bundle), and it lists some bugs that people have run into with React Native core's polyfill.

[chrisbobbe]

Ha — in fact, a React Native maintainer closed an issue, at facebook/react-native#16434 (comment), citing react-native-url-polyfill as (apparently) the official workaround.

[chrisbobbe]

I think facebook/react-native#25717, and probably others, listed there, is enough to say that we shouldn't use React Native's polyfill.

Consider the bug in #3567 (comment):

webAuth.openBrowser(`${this.props.realm}/${url}`, otp);

incorrectly doubles a slash if url (which is a .login_url on an item in .external_authentication_methods from /server_settings) starts with a slash:

https://chat.zulip.org//accounts/login/social/google

All the ones I've seen do, but we shouldn't expect that they always will. If it's missing, then facebook/react-native#25717 means we'll get

https://chat.zulip.orgaccounts/login/social/google

from

new URL(url, this.props.realm).toString()

.

[chrisbobbe]

Hmm, I don't think we can use any existing version of react-native-url-polyfill, though, until RN v0.61 (that's #3781).

This file in react-native-url-polyfill, which was added in 1.0.0, the initial release, depends on this file in React Native, which was added in v0.61.0.

On RN v0.59.10, which we're using, we get this error from Metro:

error: bundling failed: Error: Unable to resolve module `react-native/Libraries/Blob/NativeBlobModule` from `/Users/chrisbobbe/dev/zulip-mobile/node_modules/react-native-url-polyfill/js/URL.js`: Module `react-native/Libraries/Blob/NativeBlobModule` does not exist in the Haste module map

[chrisbobbe]

Hmm, I don't think we can use any existing version of react-native-url-polyfill, though, until RN v0.61 (that's #3781).

Looks like the maintainer is quite responsive, though, which is a good general sign! charpeni/react-native-url-polyfill#154 (comment)

[rk-for-zulip]

Rather than shipping a JavaScript library, would it make sense for us to make a small native plugin which just wraps an NSURL (iOS) or URL (Android) object?

[chrisbobbe]

Rather than shipping a JavaScript library, would it make sense for us to make a small native plugin which just wraps an NSURL (iOS) or URL (Android) object?

Hmm, I wonder! Some quick Googling hasn't turned up anything off-the-shelf that does this. One plausible explanation is because it's easy enough to do on one's own.

One question I'd have is whether we'd expect to spend a lot of time reconciling differences between the iOS and Android APIs.

Also, all of its methods that we call from JS would have to be asynchronous, right, because the React Native bridge is asynchronous. Might this make some race conditions hard to rule out (even if they're unlikely)?

[rk-for-zulip]

One question I'd have is whether we'd expect to spend a lot of time reconciling differences between the iOS and Android APIs.

I doubt it. The syntax and components of a URL are largely specified by RFC 3986. There are a fair number of idiosyncratic auxiliary functions on both of those classes, but we probably don't care about those.

Also, all of its methods would have to be asynchronous, right, because the React Native bridge is asynchronous.

Oh right. I think that answers my previous question ("would it make sense?") with a solid "no" – race conditions or not. 😓

[gnprice]

Hmm, I don't think we can use any existing version of react-native-url-polyfill, though, until RN v0.61 (that's #3781).

Looks like the maintainer is quite responsive, though, which is a good general sign! charpeni/react-native-url-polyfill#154 (comment)

Indeed!

At a quick look at the implementation, it looks like most of the work happens inside here:
https://github.com/charpeni/whatwg-url
(same maintainer's fork of another project). So we could probably just import that directly and call it explicitly -- it just won't be named URL.

(Or anyway it shouldn't be polyfilled as the global URL, because it won't have the createObjectURL implementation that that polyfill library adds -- and that I'm guessing RN's own implementation has, and RN relies on.)

[chrisbobbe]

Makes sense!

Something that react-native-url-polyfill (and the whatwg-url-without-unicode you mentioned, if we use it with React Native) depends on is assigning an export of buffer to the global variable Buffer, where, previously (at least in our current master) nothing is assigned. React Native core's buggy polyfill for URL has nothing to do with Buffer.

What do you think about that?

Also, we'd need to get a Flow libdef working for react-native-url-polyfill. Sadly, FlowTyped doesn't seem to have one, even for whatwg-url, which whatwg-url-without-unicode is forked from.