Fixed missing encoding via URLSearchParams

[scarlac]

Fixes #28966

Summary

URLSearchParams was largely useless, as it did not encode the parameters at all. This PR simply encodes the parameters as per the standard.

Changelog

[General] [Fixed] - URLSearchParams now URL encodes passed values

Test Plan

Ran the new code using jsc (/System/iOSSupport/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/Helpers/jsc)

_searchParams=[['size', 42],['limit', false],['baz','boing+foo&bang']]; last = this._searchParams.length - 1;
_searchParams.reduce((acc, [key, value], index) => {
  return `${acc}${key}=${encodeURIComponent(value)}${index === last ? '' : '&'}`;
}, '');
// which yields: 
// 'size=42&limit=false&baz=boing%2Bfoo%26bang'

And compared it running similar code but with a browser-shipped URLSearchParams in the browser console of this page:

_searchParams=[['size', 42],['limit', false],['baz','boing+foo&bang']]; last = this._searchParams.length - 1;
new URLSearchParams(_searchParams).toString()
// which yields:
// "size=42&limit=false&baz=boing%2Bfoo%26bang"

Comparing the two:

'size=42&limit=false&baz=boing%2Bfoo%26bang' === "size=42&limit=false&baz=boing%2Bfoo%26bang"
// returns true

[analysis-bot]

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

Base commit: b5b4a70

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,208,597	-20
android	hermes	armeabi-v7a	6,857,765	-24
android	hermes	x86	7,643,225	-20
android	hermes	x86_64	7,534,177	-12
android	jsc	arm64-v8a	9,368,250	28
android	jsc	armeabi-v7a	9,009,554	36
android	jsc	x86	9,230,974	36
android	jsc	x86_64	9,808,118	28

Base commit: b5b4a70

[charpeni]

URLSearchParams has the same issue as URLas the implementation inside React Native is all hardcoded and not following the spec.

I would highly recommend migrating over to react-native-url-polyfill instead.

Actually, I should deprecate React Native's URL and URLSearchParam.

[scarlac]

@charpeni I'm not sure what you are saying. Nothing is hard-coded about the PR or implementation and this solves an immediate issue with have the implementation. Unless we can integrate a polyfill from react-native-url-polyfill that ships with React Native, I don't see the point of removing it. We'd be needlessly breaking a lot of code bases for no good reason. The feature is helpful it just has this 1 very important bug.

[charpeni]

Hey @scarlac.

Sorry for the confusion here, your pull request is definitely the right implementation.

The hard-coded things are coming from React Native itself which is trying to define its own implementation of URL and URLSearchParams. Unfortunately, this process can't scale well and we'll always have issues with unhandled cases and corner cases. (See: #26019, #25717, #24428, #16434). As others pull requests, we can't assert that this pull request isn't breaking any existing code (relying on the bad behavior), and merging PRs that are fixing our custom implementation is a back and forth process.

I'm not against merging this PR, though. I think it's a safe™ and nice fix, just flagging the issue with those.

What I'm advocating for, is to deprecate RN's URL and URLSearchParams in favor of react-native-url-polyfill (which is spec compliant). Of course, as you said we can't remove them right away, but we can display a LogBox warning saying that it is deprecated.

Why we can't ship React Native with react-native-url-polyfill.

How does that sound?

[scarlac]

@charpeni Yeah it looks like it's ~40k extra. I don't like it but it's a reasonable trade-off. Agree with your approach then. Thanks for the thorough explanation.

[scarlac]

Closing this PR. As per @charpeni 's recommendation an app-installed polyfill is preferred and we should be removing the existing implementation.