Skip to content
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.

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

Migrate to @expo/whatwg-url to fix SharedArrayBuffer issue. #25005

Merged
merged 12 commits into from
Oct 26, 2023

Conversation

EvanBacon
Copy link
Contributor

@EvanBacon EvanBacon commented Oct 24, 2023

Why

whatwg-url-without-unicode hasn't been updated in a while and it uses an older version of whatwg-url -> webidl-conversions that doesn't support environments that are missing SharedArrayBuffer (support was added back here). To fix, I forked the latest whatwg-url to @expo/whatwg-url and stripped unicode support (pending native integration).

Test Plan

  • iOS and Android tests should no longer fail.

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Oct 24, 2023
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Oct 24, 2023
@tsapeta
Copy link
Member

tsapeta commented Oct 24, 2023

^ but please fix the SDK check 😄

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Oct 25, 2023
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
@marklawlor
Copy link
Contributor

marklawlor commented Oct 25, 2023

I'm happy to maintain a fork, but I'll tag @charpeni in case he is interested in updating whatwg-url-without-unicode and we can switch back. While whatwg-url-without-unicode hasn't been updated, the parent library react-native-url-polyfill is still active, so he may be planning on updating it in the future.

@charpeni
Copy link
Contributor

charpeni commented Oct 25, 2023

Thanks for the ping!

Both libraries are still active. I tried to update whatwg-url-without-unicode in different branches recently, but I'm always facing weird issues because the latest version of whatwg-url isn't really meant for non-web environments. If I remember correctly, it had to do with the lack of AsyncIterator support on React Native.

To fix, I forked the latest whatwg-url to @expo/whatwg-url and stripped unicode support.

That's exactly the role of whatwg-url-without-unicode and react-native-url-polyfill, it would be good to align initiatives around URL and prevent creating another fork, if possible. 🙏

Happy to work on anything that would allow Expo to use react-native-url-polyfill!

@ide
Copy link
Member

ide commented Oct 25, 2023

AFAIK react-native-url-polyfill already works with Expo. Separately, the change in this PR is a step towards adding URL as a built-in to Expo core rather than polyfilling it. However, it could make sense to collaborate on upstreaming some of the code in react-native-url-polyfill into the Expo runtime, though I have not looked at the code specifically.

@charpeni
Copy link
Contributor

👋 James

Ah, perfect, cause I have a bunch of tests in react-native-url-polyfill to make sure it is well tested against Expo. ❤️

I read all the commits, and that's the gotcha that happened, the initial implementation used a beta version of whatwg-url-without-unicode (one of my experiments to use the latest version of whatwg-url that failed because of SharedArrayBuffer).

I also see that it was reverted (89dff0c) in favor of the latest non-beta version of whatwg-url-without-unicode, which should be well-supported even though the implementation of whatwg-url is older. Using whatwg-url-without-unicode@8.0.0-3 should be perfect for Expo core and should prevent the need to fork into @expo/whatwg-url.

If you would really like to have the latest implementation of whatwg-url, I can find more time to dig into that, but I'm sure we'll also need to polyfill a bunch of other things too, which is growing to the size of the bundle. 😅 The maintainer of whatwg-url was quite explicit that it isn't meant for non-web platforms and therefore it's been quite tedious to follow up.

@EvanBacon
Copy link
Contributor Author

@charpeni yes the latest version of whatwg-url requires at least SharedArrayBuffer, async iterators, and TextEncoder/TextDecoder (we're investigating adding encoding natively too).

@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against 88ec040

@EvanBacon EvanBacon merged commit 13f796f into main Oct 26, 2023
16 of 18 checks passed
@EvanBacon EvanBacon deleted the @evanbacon/expo/use-whatwg-url-expo branch October 26, 2023 03:46
marklawlor pushed a commit that referenced this pull request Oct 30, 2023
# Why

`whatwg-url-without-unicode` hasn't been updated in a while and it uses
an older version of `whatwg-url` -> `webidl-conversions` that doesn't
support environments that are missing `SharedArrayBuffer` (support was
added back
[here](jsdom/webidl-conversions@0449cdd)).
To fix, I forked the latest `whatwg-url` to `@expo/whatwg-url` and
stripped unicode support (pending native integration).

# Test Plan

- iOS and Android tests should no longer fail.

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants