Finish replacing ad-hoc heuristic URL string processing

[chrisbobbe]

Now that #4081 has landed, adding the react-native-url-polyfill package, we can finish using URL objects instead of string concatenation and regexes! The thing it's polyfilling is this: https://developer.mozilla.org/en-US/docs/Web/API/URL (that's a handy and reliable doc from MDN).

See 8f0f5aba8 for an example of how to do this in a simple case.

One good place to start would be getFullUrl in url.js. I'm optimistic that we can replace all of its callers with something more transparent, using this URL API, and dissolve getFullUrl. If not, then it should at least be made much clearer and bug-free.

We'll also want to look closely at every helper function in src/utils/url.js. Most of them aren't tailored to logic specific to Zulip, which is a hint that there's been some reinvention of the wheel that we won't have to do anymore.

There are likely a few idiosyncratic cases that don't even use the helpers in src/utils/url.js. We'll want to track down all of these and fix them.

I think it wouldn't be a bad idea for, e.g., realm in Account objects, to use a URL object instead of a string. A URL instance is not serializable, so we'd need correct replace/revive logic, as in bfe794955 and following.

---

(See also update below: https://github.com/zulip/zulip-mobile/issues/4146#issuecomment-747920485 )

[agrawal-d]

Does this polyfill contain exported functions for decoding/encoding links? If so, this can also fix https://github.com/zulip/zulip-mobile/issues/4136.

[chrisbobbe]

I just added a link to an MDN doc for the thing this is polyfilling, which should help answer questions like these. 🙂 Here's one quote:

URLs are encoded according to the rules found in RFC 3986.

I'm not sure this will fulfill what @gnprice describes at https://github.com/zulip/zulip-mobile/issues/4136#issue-631242811, though:

So it seems like we need to, in openLink on iOS before passing the URL to SafariView.show:

• %-encode non-ASCII characters -- that's #3315;
• but not %-encode % itself, instead leave it alone;
• and presumably also leave alone all the other characters that encodeURI leaves alone, like a and Z and /;
• and it's not clear if we should %-encode the remaining characters that encodeURI affects, like ` and"` and a bunch of other punctuation.

I'm wondering whether this selective encoding of some things and not others might introduce its own bugs? Probably not larger ones that those we've described so far, though.

[chrisbobbe]

Oh—I guess now that we've decided to use react-native-url-polyfill instead of just whatwg-url-without-unicode, it'll be fine to use Option 1 of the setup, described here—i.e., mutate the URL global variable. At https://github.com/zulip/zulip-mobile/issues/4081#issuecomment-623716266, @gnprice pointed out that it would be bad to do this if we were just using whatwg-url-without-unicode.

React Native has already put their buggy polyfill at the URL global, so leaving that in place without overwriting it will probably cause confusion, and bugs if people don't remember to import URL from react-native-url-polyfill.

[gnprice]

Since commit 413bf9521df8887325179e004a9d91e634120a56, we set the URL global to this polyfill. So we can create URL objects by just saying new URL(url) or new URL(url, base), as described in MDN.

[gnprice]

We (re-)discovered this week that the URL implementation we've polyfilled is rather expensive. After introducing several calls to new URL per user in the initial load, we had to promptly take them back out again (#4344 and #4345) because they were adding over 7 seconds to the initial load time on chat.zulip.org.

For a more concrete figure: the first commit of #4344 removed one new URL call on each applicable user, probably the bulk of the 17400 users, and it saved about 4.5s. That comes to >0.25ms per call (and more than that, if it applied to a fraction much less than 100% of users.)

This means that in places where we're potentially handling a large number of URLs, we'll need to be cautious about using new URL. It's still very helpful that we use it for, e.g., the realm URL, which there are very few of. It's also perfectly fine to use in places where we'll be doing so just one or a small handful of times in a whole interaction initiated by the user, like when they're following a link.

And where we don't ultimately use new URL, we still want to do something where we can clearly describe what it's intended to mean, in terms of the URL Standard, and try to justify confidence it really does mean that by both carefully reading the spec, and writing tests with challenging test data. That's a standard that a lot of the existing helpers in src/utils/url.js do not meet.