Skip to content

Add react-native-url-polyfill. #4145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 9, 2020
Merged

Add react-native-url-polyfill. #4145

merged 2 commits into from
Jun 9, 2020

Conversation

chrisbobbe
Copy link
Contributor

Fixes: #4081

I'll follow this up by creating an issue to fix the many uses of regexes and string concatenation; I think #4081 was mainly for settling on a workable solution.

@chrisbobbe chrisbobbe requested a review from gnprice June 9, 2020 19:08
chrisbobbe added 2 commits June 9, 2020 12:39
@chrisbobbe @gnprice
deps: Add react-native-url-polyfill.
c675cdd 
React Native does have its own built-in polyfill for the URL Web API
[1], but it's so minimal that it has some serious bugs, and a React
Native maintainer has even recommended `react-native-url-polyfill`
as an official workaround for one of those bugs [2].

Many thanks to charpeni for the changes [3] that allow RN v0.60
support! And for the changes from our own PR
charpeni/whatwg-url#3, to avoid touching the
Buffer global.

This commit uses a release candidate version; there will likely be
an official release soon.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/URL

[2]: facebook/react-native#16434 (comment)

[3]: charpeni/react-native-url-polyfill@d4e26ce#diff-375a2b7b7d929526f0dd58cd8bb8665b

Fixes: zulip#4081
@chrisbobbe @gnprice
AuthScreen: Use react-native-url-polyfill for the first time.
8f0f5ab 
This is a straightforward case where it's easy to see what the
RegExp was doing, and that work is easily done by
`react-native-url-polyfill`.
@gnprice
Copy link
Member

gnprice commented Jun 9, 2020

Thanks!

What's the origin of the libdef -- is that from... ah, I see, there are links in comments.

Those links are to libdefs built into Flow, for the types URLSearchParams and URL. Hmm, it sure would be nice to just refer to those, if that's the effect we want, rather than copy them. But then I'm not sure how to actually spell that, because the names are the same.

Nit in commit message:

Many thanks to charpeni for the changes [2] that allow RN v0.60

I think that's meant to be [3].

Otherwise LGTM. I'll fix that one nit and merge.

@gnprice gnprice force-pushed the pr-url-polyfill branch from 57dcb40 to 8f0f5ab Compare June 9, 2020 19:50
@gnprice gnprice merged commit 8f0f5ab into zulip:master Jun 9, 2020
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jun 9, 2020 •
edited
Loading

Thanks for the review, fix, and merge!

What's the origin of the libdef -- is that from... ah, I see, there are links in comments.

Those links are to libdefs built into Flow, for the types URLSearchParams and URL. Hmm, it sure would be nice to just refer to those, if that's the effect we want, rather than copy them. But then I'm not sure how to actually spell that, because the names are the same.

Yeah. I tried a few things to use those builtins, but nothing's worked so far.

@chrisbobbe chrisbobbe deleted the pr-url-polyfill branch November 6, 2020 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice Awaiting requested review from gnprice

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Start using proper URL objects instead of string concatenation
2 participants
@chrisbobbe @gnprice