Warn about URL and URLSearchParams not being spec-complaint

[charpeni]

Summary

Fixes #16434
Fixes #24428
Fixes #25717
Fixes #26019
Fixes #29834
Fixes #30152

Follows up on #25719.

The current implementation of URL and URLSearchParams included in React Native is a homemade polyfill that doesn't
follow the URL Standard. It was done like this to provide a lightweight solution to URL and URLSearchParams without affecting the bundle size.

It can't be done directly in React Native, as this is affecting the bundle size, see: #25719 (comment).

So, let's deprecate warn about React Native's implementation of URL and URLSearchParams not being spec-compliant and refer to react-native-url-polyfill (~255k monthly downloads) instead. It's a spec-compliant implementation(minus Unicode support) of the WHATWG URL Standard optimized for React Native, with Blob and Hermes support, backed by unit tests, and Detox e2e tests.

Even though the warning says it will be removed, I don't think we should remove it soon as it will be a significant breaking change. Instead, let's just make sure to warn people that are using RN's implementations and refer them to proper implementation. We should avoid a lot of surprises for developers trying to understand what's going on with their URLs.

Changelog

[General] [Deprecated] - Warn about URL and URLSearchParams not being spec-complaint

Test Plan

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,705,335	+0
android	hermes	armeabi-v7a	7,236,558	+0
android	hermes	x86	8,126,335	+0
android	hermes	x86_64	8,091,316	+0
android	jsc	arm64-v8a	9,624,993	+0
android	jsc	armeabi-v7a	8,543,744	+0
android	jsc	x86	9,640,286	+0
android	jsc	x86_64	10,248,954	+0

Base commit: be53728

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: be53728

[acostalima]

Subscribed. I'm looking forward to the outcome of this. I did have a look #25719 and it seems a spec-compliant URL and URLSearchParams are not going to make it to the core any time soon, right?

[charpeni]

No, it won't make it to the core.

[cpojer]

I'm not sure I'm on board with this deprecation message. I'm sure everyone uses this in their stack somewhere and showing this warning to everyone on every load seems like a bad experience, as is asking them to increase app size even if they may not need the full spec, especially when we have no plans to remove this API. I'm unclear on what our path forward should be on this, but a loud warning like this doesn't seem to be it.

[charpeni]

Hey @cpojer! 👋 Thanks for having a look at this PR.

I agree that it seems vocal, but on the other hand, we were not sure in the last PR if people were actually using URL or URLSearchParams. Even though it seems intrusive, I think it's good to be vocal about it, so people can move away with ~3 lines changes instead of using the half-baked implementation. If we do add a note in the next release note and be vocal about it on Twitter, we'll probably get
some people over the spec-compliant implementation, but unfortunately, we won't reach all users.

Currently, there's a lot of issues with URL and URLSearchParams as reported in the pull request description and I think those issues are doing more harm than a warning. Having the half-baked implementation being unpredictable (like new URL() not working as expected) seems like a bad experience to me, not the annoying warning. 😕

If we are annoying people with it, then they're using it and they should move away. If we are annoying them and they don't want to move away because of the impact on the bundle size, well they'll probably use YellowBox to opt-out anyway.

The best-case scenario will be to be vocal about it so people can easily use a spec-compliant solution, they don't have to change imports or their code, they can install the dependency and use the auto polyfill.

If we want to be less intrusive with the warning (note that we're currently using warnOnce), can we use something like AsyncStorage to make sure to only warn once by application lifetime?

Please let me know how you feel about it and what we can do to improve the developer experience around URL and URLSearchParams. 🙏

[leethree]

I agree with @charpeni. It seems to me this URL implementation is intended for use internally in React Native and never meant to be used as a spec-compliant API. And it should be very clear to whoever accidentally uses this API.

Doing nothing for three years because "we have no plans to remove this API" just doesn't make sense to me. These APIs are fundamentally broken because they can't even handle the most basic use cases. Broken APIs are worse than in-your-face warnings or increased bundle size.

If there's no plan to remove it, I suggest we change the warning message to something like "This API is incomplete. Use at your own peril." Users can always suppress it using LogBox.ignoreLogs() if they want to live dangerously.

[acostalima]

If we want to be less intrusive with the warning (note that we're currently using warnOnce), can we use something like AsyncStorage to make sure to only warn once by application lifetime?

I think this approach could work as it is as much informative and less intrusive, indeed.

I'm not exactly fully aware of the extent of React Native's documentation in this regard, but, as far as I know, it's not documented anywhere today (please correct me if I am wrong). Perhaps more detailed information could be added to the docs about the specifics around what web and core APIs are supported, whether they are spec-compliant, what limitations they have and what are the known solutions. The proposed warning message could go along with a link to such page and section of the docs.

The IPFS project, namely the HTTP client package which is the piece we're currently testing, does not work at all in React Native mostly due to unsupported or non-spec compliant Web APIs. I was running blind at first but after a few debug sessions I took note of several issues. Had I known in advance or realize what kind of support there was in the environment, I'd have been better prepared to handle the situation. In fact, if documented somehow, I think React Native developers overall will have their DX significantly improved by having necessary knowledge on how to proceed in the face of these issues around API support.

[lunaleaps]

Since it doesn't seem like we're really deprecating this library, how about we change the error messages to either document or link to documentation about the limitations of this API and solution if you're limited by it (using the library react-native-url-polyfill).

And to clarify what I mean by error messages, I mean the ones that are being modified here: #30152 so we're not proactively making noise until its necessary. Would that work?

[charpeni]

Thanks for circling back @lunaleaps. The issue with URL and URLSearchParams isn't exactly the limitations themselves, but the fact that current functionalities aren't working as expected and therefore isn't spec-compliant with the URL Standard which could (and will!) produce unexpected behaviors that are hard to track down.

I would argue that no one should be using React Native's URL and URLSearchParams. I don't think we should remove them because they will be breaking a significant number of applications, but I strongly believe we should warn consumers to provide a friendly developer experience. Having to track those URL issues down isn't fun at all, I didn't want to believe that the bundled polyfill was wrong at first. 😅

With this pull request, we're only making noise if you're using React Native's URL and URLSearchParams.

I would love to hear your thoughts on that and how we can iterate on this matter so we can ship a better developer experience to React Native's users.

[lunaleaps]

@charpeni
I see, yea definitely a gotcha!

If we're warning once on usage that works for me. I think only issue now is the language of this warning. We aren't deprecating this behavior so maybe we can change that to some documentation about why this isn't spec complaint and the trade-offs of that and links to use a spec-compliant version. How does that sound?

[charpeni]

@lunaleaps I like that! I’ll update this pull request with a better copy.

[lunaleaps]

Maybe we can update the title too, maybe like Warn about URL and URLSearchParams not being spec-complaint

[lunaleaps]

With some discussion with @yungsters, and reading your discussion with @cpojer, why wasn't the injection option (of the stripped down what-wg) pursued? I think that seems like it gives everyone the flexibility and of course the option to pull-in the full if you need it?

[charpeni]

From what I recall, back in the days, Metro wasn't supporting optional imports, dynamic imports, tree-shaking, or dead code elimination. So, on paper injecting polyfills if needed seems like the best option, but because of the limitations, polyfills and their dependencies were always ending up in the bundle even when they were not used.

The current state of React Native's bundlers has changed a bit since then, but as there are multiple options (Metro, Haul, Repack, etc.) we can't be sure what will be the outcome (polyfills included or not), even if optional imports are now supported in the latest versions of Metro. Of course, they're some ways to bypass this, but the developer experience would feel like weird quirks. I would love the ability to configure polyfills based on whether they're getting used or not, but it will require additional work related to tree-shaking and dead code elimination.

So, we just went with the external library (react-native-url-polyfill) that is heavily tested and can be opt-in with a convenient single liner: import 'react-native-url-polyfill/auto';, which is significantly a better developer experience.

In the future, I think we should work on the ability to opt-in/opt-out from polyfills based on detected usages in the bundle. Being able to automatically do that would provide security for developers instead of manual opt-in based on awareness, as calls to URL and URLSearchParams could be coming from a third-party library. A good example would be an API library calling to something like: new URL('api', 'https://google.com'). Developers may not be aware of what's happening under the hood, but the library would expect the URL to be correctly formated, but with the current React Native's implementation, it will end up being https://google.comapi.

[Nantris]

Since it doesn't seem like we're really deprecating this library, how about we change the error messages

Just my two cents, URL should be on the way to spec-compliance or should be deprecated. There's no middleground here in my view.

[charpeni]

I rewrote the warning:

[charpeni]

@lunaleaps Any chances we could get this in RN 0.66?

[lunaleaps]

@charpeni Sorry for the delay -- was out of office this past week.

I'm not sure we should pick this into 0.66 right now. We are working on the release process overall and hopefully get our releases out more consistently.

Re: the question of selective injection -- could it be a static configuration an app-owner can modify to exclude/include?

@rubennorte had some thoughts as well about options -- I'll let him share.

[rubennorte]

Thanks @charpeni for looking into this.

Just my two cents, URL should be on the way to spec-compliance or should be deprecated. There's no middleground here in my view.

I agree with this, and I think there's some consensus in the team about aligning more with Web standards in the future, so we should provide a spec-compliant version out of the box.

Unfortunately, just replacing the current API with a spec-compliant version would cause a regression on app size because we unconditionally require the default initialization module (InitializeCore) in all renderer implementations (e.g.: https://github.com/facebook/react-native/blob/main/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js#L15).

What I think we should do is:

1. Stop importing the initialization module automatically and move that to the beginning of the app bundle in userland. This will ensure the runtime is properly initialized from the very beginning (and not just after the renderer has been called) and will also allow users to customize what they want to include from the standard library (e.g.: in the case of Facebook, we might want to include this custom URL implementation instead of the default one).
2. Replace the default implementation with a spec-compliant version, as done in Implement URL standard and URLSearchParams spec compliant #25719.

Both these steps would be breaking changes but we can apply them at the same time.

Thoughts?

cc @lunaleaps , @yungsters

[lunaleaps]

@rubennorte That sounds good with me -- good communication about this will be important. We'll probably want to either write something about this or specifically highlight in release notes

[rubennorte]

@rubennorte That sounds good with me -- good communication about this will be important. We'll probably want to either write something about this or specifically highlight in release notes

Yeah, I agree. For existing projects, we need to make sure people know they need to import a new module in their bundle files. For new projects, I think it should be enough with updating the templates (so we need to communicate this to template owners, including our own template, Expo, etc.).

[rubennorte]

Stop importing the initialization module automatically and move that to the beginning of the app bundle in userland. This will ensure the runtime is properly initialized from the very beginning (and not just after the renderer has been called) [...]

I'd like to clarify something after I looked at this more closely. Apps using the React Native CLI and Expo are already injecting and executing InitializeCore before the EntryPoint of the bundle, so we can actually just remove InitializeCore from the renderer because it's a no-op.

[lunaleaps]

Stop importing the initialization module automatically and move that to the beginning of the app bundle in userland. This will ensure the runtime is properly initialized from the very beginning (and not just after the renderer has been called) [...]

I'd like to clarify something after I looked at this more closely. Apps using the React Native CLI and Expo are already injecting and executing InitializeCore before the EntryPoint of the bundle, so we can actually just remove InitializeCore from the renderer because it's a no-op.

But for apps that don't? I guess breaking change for them only?

[rubennorte]

Stop importing the initialization module automatically and move that to the beginning of the app bundle in userland. This will ensure the runtime is properly initialized from the very beginning (and not just after the renderer has been called) [...]

I'd like to clarify something after I looked at this more closely. Apps using the React Native CLI and Expo are already injecting and executing InitializeCore before the EntryPoint of the bundle, so we can actually just remove InitializeCore from the renderer because it's a no-op.

But for apps that don't? I guess breaking change for them only?

@lunaleaps apps that don't use the CLI or Expo most likely had that in their Metro configurations as well, but yeah, it could happen that they didn't. We can share this is a breaking change with a note that for most users it actually won't, so we just do it out of caution.

[github-actions]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This PR was closed because it has been stalled for 7 days with no activity.

[Nantris]

This doesn't seem like it should be closed. @lunaleaps