-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Implement URL standard and URLSearchParams spec compliant #25719
Conversation
|
@charpeni did you make any progress on this? |
@cpojer Yes! Jungling with other priorities at the moment, but I should be able to push changes soon. |
After looking at this, our best approach is to use Here are some stats: 📦 Then, As This PR is ready to review! |
Thanks for doing this exploration. Adding 60k to bundle size for something that not everyone needs seems excessive. At Facebook we are trying to reduce the size of our apps and it will be hard to justify adding this much code at this time. What if we made the URL implementation injectable? That way people could modify the URL class that is used, like |
Thanks, @cpojer. I would have loved getting that concern during the draft PR 😅. I totally agree that adding something to the bundle size for something that not everyone needs is excessive. Adding the initial 352 KB is excessive, but adding the forked version of 53 KB doesn't seem that excessive considering the fact that I'd be afraid that React Native consumers will still have weird behavior without acknowledging that it could come from our
If we keep our current implementation with a possibility to inject one at the choice of users, it would be the same frustration as now:
I'd be more prompt to fix our implementation even regarding the additional 53 KB to the bundle size than shipping something that is not polished/usable to React Native consumers. Or, do a breaking change by removing it. The initial 352 KB was excessive, but 53 KB to solve those issues seems to worth a discussion. If we want to focus on reducing the bundle size, I think that focusing our efforts on tree shaking within Metro would be beneficial for every dependency that not everyone needs. Like by removing Otherwise, making them all injectable could be helpful? Not sure. 🤷♂ What do y'all think? Would love to hear other feedback on the bundle size in general and |
My previous numbers were the minified size from bundlephobia. I did some tests while playing with 📦 RNTester's bundle, here are my observations:
|
@charpeni sorry for not raising it earlier. I didn't realize that after my comment you would immediately get back to iterate on this and I wasn't sure which solution you were picking. That's on me. I am pointing out that adding 50k of compressed app size to fix hypothetical use cases is a very very hard sell at Facebook right now. The reason I suggested the approach with injection is because we could document it and only the people who need this will have to deal with a bigger bundle size. In that case we can recommend people to use the official whatwg-url solution as well. |
@cpojer if we make it injectable, what do you think about doing a breaking change by completely removing |
Interesting question, I think that could work. I unfortunately don't have a good idea on how much URL is used within RN itself and how many users depend on it. How can we assess the impact of such a breaking change? |
@cpojer I'm sorry but I cannot agree with your assessment that proper URL handling is "hypothetical use case" when you cannot even handle the most basic use case like The React Native team should either make |
What we all want is a way to have polyfill for missing classes, right? If we split the apple in half, we could say that we all want to configure the polyfills we actually need without adding unnecessary code to the bundle. But, we won't have to deal with installing and applying polyfills ourself. What do y'all think if we expose something like this from React Native: configurePolyfills({
URL: true,
URLSearchParams: true,
fetch: false,
}); Then, the corresponding polyfill will be added to global or not, based on this or on the RN's default value. React Native will provide expected polyfills within its dependencies`, but may not include them by default, which won't affect the bundle size, right? react-native/Libraries/Core/setUpXHR.js Lines 31 to 32 in a5ea720
We could let the legacy implementation of |
Closing in favor of https://github.com/charpeni/react-native-url-polyfill. |
Fixes #25717
Fixes #24428
Fixes #16434
Summary
Currently, our implementation of
URL
is not spec-compliant and this is causing headaches to RN users.Tests were added to
URL
, to be sure we're spec compliant with https://url.spec.whatwg.org/.As a solution to those issues, we could do one of the following:
whatwg-url
instead of our custom implementation.whatwg-url
and remove the support of unicode from there, becausetr46
dependency is like ~80% ofwhatwg-url
size.I'll tackle an implementation of
whatwg-url
and compare what will be the increase of RN size with and without unicode support.Let me know if you have any thoughts, concerns or ideas.
After looking at this, our best approach is to use
whatwg-url
to cover all the spec. But we can't usewhatwg-url
directly, because it's coming with a dependency totr46
to handle Unicode cases. I think we can live without Unicode, so I forkedwhatwg-url
, and I removed the unicode support, edited tests, and published this aswhatwg-url-without-unicode
.Here are some stats:
📦
whatwg-url
is taking 352.5 KB.📦
whatwg-url-without-unicode
is taking 53.5 KB.Then,
whatwg-url-without-unicode
requiresBuffer
, which is 142 B.As
URLSearchParams
is bundled inwhatwg-url
, I replaced our custom implementation with the spec-compliant one and some tests to cover this.Changelog
[General] [Changed] - Implement spec compliant URL and URLSearchParams
Test Plan
Added tests, ran RNTester, and called
new URL()
.