[RNMobile] Fix isURL

[etoledom]

gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1879

Description

The changes on the PR #19871 to isURL assume that the constructor of URL "throws an error for an invalid URL", which is not the case for the mobile implementation.

This PR brings back the previous implementation of isURL just for native, with a few improvements to make it behave as the new web version (tested running the same set of tests).

@SergioEstevao - I remember you having some opinions about the isURL check, could you please take a look at this?

Also @aduth - since you authored the original PR, it would be great if we could have a shared implementation on this, but from what I see, we can not trust that the URL constructor will throw.

Any improvement to the given solution is welcome!
The implemented Regex was taken from here, adding a group to match for ports.

How has this been tested?

• The new native Unit Tests should pass.
• Test on the related gutenberg-mobile PR following the given test steps.

[aduth]

Also @aduth - since you authored the original PR, it would be great if we could have a shared implementation on this, but from what I see, we can not trust that the URL constructor will throw.

In putting together #19871, I was careful to consider that validation conform to the contract of what is considered valid per the URL standard, and only incidentally that this be fulfilled using the URL constructor. For how it impacts here, I agree that it would be nice if the implementation would be shared, but strictly speaking it does not need to.

That said:

• I feel that React Native's implementation of URL is not spec-conformant. The specification dictates that "if parsedURL is failure, then throw a TypeError". I observe that the React native implementation of URLSearchParams includes some parts of jsdom/whatwg-url, where that package's implementation of URL does throw. This doesn't impact how we decide to proceed for the short-term, but it might be something to report for upstream improvement.
• Would it be an option for the native implementation to use one of either the jsdom/whatwg-url, or core-js's implementation, or the URL polyfill we use for unsupported browsers instead, at least to be equally spec-conforming?
• I can grant that the changes in this pull request would at least be the minimal changes necessary to restore some workable behavior, but I would not be comfortable with the diverging expected outputs as a long-term solution.

[aduth]

• Would it be an option for the native implementation to use one of either the jsdom/whatwg-url, or core-js's implementation, or the URL polyfill we use for unsupported browsers instead, at least to be equally spec-conforming?

I'm glad I brought this up, because in closer inspection of the polyfill we chose to use for URL, it also does not appear to throw errors as we are relying on it to do for URL-checking.

Edit: Reported upstream at https://github.com/Financial-Times/polyfill-library/issues/462

[aduth]

See also: https://github.com/charpeni/react-native-url-polyfill

Specifically: https://github.com/charpeni/react-native-url-polyfill#why-do-we-need-this

Official (?) stance: facebook/react-native#16434 (comment)

[etoledom]

Thank you @aduth for your feedback! I've tested using react-native-url-polyfill and it seems to do the job! 🎉

Updated the PR with this change. I also added this package as deb dependency to gutenberg to run the native unit tests properly.

[aduth]

Looks sensible from my perspective 👍

I'll leave it to your discretion whether it needs eyes from one more familiar with mobile side of things.

[aduth]

Actually, can you confirm: Should react-native-url-polyfill be defined as a dependency of the url package rather than in the root package.json? Typically I would expect to see a package's own dependencies listed only in its own package.json, not at the root. Unsure if this is different for React Native work.

[etoledom]

@aduth - some updates: react-native-url-polyfill is giving us troubles on Android with the current setup. It crashes with this error:

Talking with @hypest internally, we have decided to go back to the first proposal momentarily, in order to fix wordpress-mobile/gutenberg-mobile#1875, and then we would come back to react-native-url-polyfill as soon as the Android issue is solved.

I'll wait for your confirmation before rolling back the commits 👍

[aduth]

then we would come back to react-native-url-polyfill as soon as the Android issue is solved.

Is this a known upstream issue? Do we have a sense why the error is happening?

If it's simplest to revert to the first proposal as a fix for wordpress-mobile/gutenberg-mobile#1875, that sounds reasonable to me. I would just be wary to not let these diverge too long, since the two implementations can have differing resulting values from its input (the earlier implementation being much more strict about "http" URLs).

[etoledom]

Reverted to the previous proposal.

the earlier implementation being much more strict about "http" URLs.

@aduth - This proposal is a bit more complex than the older implementation. I was careful to make the same set of tests implemented on the web side pass. So things like:

[ 'http://wordpress.org' ],
[ 'https://wordpress.org' ],
[ 'mailto:[email protected]' ],
[ 'ssh://user:[email protected]:8080' ],

...will all return true. I hope this gives a bit more confidence. But it's true that there might be other urls that these implementations will return different results.

There is this related issue, I think it's the same problem but haven't had much attention so far. We might need to try to push it a bit.

[aduth]

If it can pass the same tests, that seems like a perfectly reasonable alternative implementation. And if it turns out not to be, then it's a clear indication that the tests could be improved. A win/win either way! (Well, except for those affected by however it might hypothetically break 😅)

Noting that this aligns to a remark I made at #19871 (comment) in the original implementation:

One advantage of having the tests is that it could support some potential future refactoring where we choose not to implement it using the URL constructor.

[Tug]

LGTM 👍

[etoledom]

Thank you both! 🎉

[charpeni]

👋 Hey!

I'm the author of react-native-url-polyfill. I flagged the issue with Hermes: facebook/hermes#193, and I fixed it in react-native-url-polyfill. Everything is now working fine with Hermes!

[aduth]

Hi @charpeni , thanks for the heads-up! The conversation of using a proper polyfill has come up again in #20537, so I might imagine it could be a good place to consider again to incorporate react-native-url-polyfill.