Improve addQueryArgs

[sgomes]

This PR aims to improve addQueryArgs, moving it to its rightful place in lib/url, and reimplementing it with native URL functionality (instead of the browserified version of node's url module).

Changes proposed in this Pull Request

• Move addQueryArgs to lib/url
• Reimplement it using native URL functionality
• Add a bunch more tests to cover all URL types

Testing instructions

The unit tests should provide some measure of comfort, complemented by the E2E tests.

[matticbot]

Test live: https://calypso.live/?branch=update/improve-add-query-args

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~1756 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-login              +2159 B  (+0.2%)     +585 B  (+0.2%)
entry-gutenboarding      +2151 B  (+0.1%)     +587 B  (+0.1%)
entry-main               +2149 B  (+0.1%)     +584 B  (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~2 bytes removed 📉 [gzipped])

name             parsed_size           gzip_size
jetpack-connect        -12 B  (-0.0%)       -2 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[ockham]

I was hoping we could drop the entire thing in favor of its counterpart from @wordpress/url (caution: argument order is flipped there, url before args) 😬

Feel like codemoddin'? 😬

[sgomes]

I was hoping we could drop the entire thing in favor of its counterpart from @wordpress/url (caution: argument order is flipped there, url before args) 😬

Feel like codemoddin'? 😬

@ockham @wordpress/url appears to use external libraries, namely qs, which is 3.5KB (compressed) by itself, resulting in a total of 4.3KB with no apparent code-splitting (all of its methods live in a single file, which makes it hard to code-split).

The goal behind this PR, previous ones, and future ones I had planned, is to replace as much 3rd party code (the deprecated browserified url module, qs, querystring, etc.) as possible with native functionality, and filling in the blanks with polyfills in older browsers. That gets us a number of benefits:

• We're using standard APIs that won't change under our feet
• We don't have to worry about security vulnerabilities in 3rd party code
• We keep bundles small for evergreen browsers, that can get the evergreen bundle with no polyfills included

As such, I'm reluctant to replace this with @wordpress/url. Is there any particular benefit in adopting it, other than code reuse? Is there any similar work to this planned on that library?

[ockham]

I was hoping we could drop the entire thing in favor of its counterpart from @wordpress/url (caution: argument order is flipped there, url before args) grimacing
Feel like codemoddin'? grimacing

@ockham @wordpress/url appears to use external libraries, namely qs, which is 3.5KB (compressed) by itself, resulting in a total of 4.3KB with no apparent code-splitting (all of its methods live in a single file, which makes it hard to code-split).

The goal behind this PR, previous ones, and future ones I had planned, is to replace as much 3rd party code (the deprecated browserified url module, qs, querystring, etc.) as possible with native functionality, and filling in the blanks with polyfills in older browsers. That gets us a number of benefits:

• We're using standard APIs that won't change under our feet
• We don't have to worry about security vulnerabilities in 3rd party code
• We keep bundles small for evergreen browsers, that can get the evergreen bundle with no polyfills included

Thanks for explaining, all of this makes sense ❤️

As such, I'm reluctant to replace this with @wordpress/url. Is there any particular benefit in adopting it, other than code reuse?

Nah, really just code reuse. Including secondary aspects that follow from that like de-duplication of functionality, single point of maintenace, etc.

Is there any similar work to this planned on that library?

Not that I'm aware of (I'm not that directly involved with Core). I mean it definitely seems like that lib would profit from an effort like this; but do feel free to proceed with this one 👍

[sgomes]

I just noticed that we are actually already using @wordpress/url in Calypso, so we won't be able to migrate away from qs anyway as long as that's the case 😞 Sigh... It's too easy to add dependencies to the project. And we may have indirect dependencies too, now that we're pulling in @wordpress components.

Is there any similar work to this planned on that library?

Not that I'm aware of (I'm not that directly involved with Core). I mean it definitely seems like that lib would profit from an effort like this; but do feel free to proceed with this one 👍

I'd be more than happy to work on @wordpress/url and make these changes there instead, but I wouldn't know where to start, both regarding whom to talk to and how to go about that effort. @blowery: would you happen to know more?

At first glance, it'd require:

• Splitting up the library source into multiple modules, to take advantage of code-splitting
• Adding the new functionality that was added to lib/url, to fill in the gaps between the old url module and the standard URL/URLSearchParams functionality
• Reimplementing all of the library to use native URL and URLSearchParams
• Making use of a URL and URLSearchParams ponyfill instead of a polyfill. A library shouldn't make global changes.
• To avoid code bloat, adding a sources directory to the package as well, so that Calypso could consume it without incurring the cost of the ponyfill (since it already uses polyfills)
• Releasing a new version of the package. Potentially a major version, if it was impossible to maintain full compatibility.

Then, on the Calypso side:

• Mapping imports from @wordpress/url to (e.g.) @wordpress/url/src, to ensure the sources are used

That's a lot of changes, and while I don't mind spending the time making them, it is a big change for the library. It feels like something that should be discussed, not just dropped into a PR all of a sudden 😅

[ockham]

I just noticed that we are actually already using @wordpress/url in Calypso, so we won't be able to migrate away from qs anyway as long as that's the case Sigh... It's too easy to add dependencies to the project. And we may have indirect dependencies too, now that we're pulling in @wordpress components.

Ah right, good point 😬

Is there any similar work to this planned on that library?

Not that I'm aware of (I'm not that directly involved with Core). I mean it definitely seems like that lib would profit from an effort like this; but do feel free to proceed with this one +1

I'd be more than happy to work on @wordpress/url and make these changes there instead, but I wouldn't know where to start, both regarding whom to talk to and how to go about that effort. @blowery: would you happen to know more?

All the @wordpress/ npm packages are in the Gutenberg monorepo: https://github.com/WordPress/gutenberg/tree/e6d9398e55f0284102cf75b6948eac4219a77fa8/packages

Other than just filing PRs against that repo, you'd need to talk to Gutenberg people, e.g. @youknowriad, @gziolo, or @aduth.

At first glance, it'd require:

• Splitting up the library source into multiple modules, to take advantage of code-splitting
• Adding the new functionality that was added to lib/url, to fill in the gaps between the old url module and the standard URL/URLSearchParams functionality
• Reimplementing all of the library to use native URL and URLSearchParams
• Making use of a URL and URLSearchParams ponyfill instead of a polyfill. A library shouldn't make global changes.
• To avoid code bloat, adding a sources directory to the package as well, so that Calypso could consume it without incurring the cost of the ponyfill (since it already uses polyfills)
• Releasing a new version of the package. Potentially a major version, if it was impossible to maintain full compatibility.

Then, on the Calypso side:

• Mapping imports from @wordpress/url to (e.g.) @wordpress/url/src, to ensure the sources are used

That's a lot of changes, and while I don't mind spending the time making them, it is a big change for the library. It feels like something that should be discussed, not just dropped into a PR all of a sudden

Yeah, makes sense. I think Core typically discusses things like that either in issues in the Gutenberg repo, Core Slack, or make.wordpress.org. (The latter might be better suited for more high-level proposals that involve design or more far-reaching technical decisions.)

[gziolo]

Other than just filing PRs against that repo, you'd need to talk to Gutenberg people, e.g. @youknowriad, @gziolo, or @aduth.

If you are able to refactor @wordpress/url to avoid using qs in the browser with native handlers in a way where all existing tests pass then go for it. This small utility library is mostly developed by the community.

[aduth]

Related Gutenberg issue: WordPress/gutenberg#13386

In truth, I only recently learned that URL was a common global available in both Node and the browser. I would be happy to see those modules refactored to use it.

[sgomes]

Thank you all! In that case, I'll be happy to take on this work in @wordpress/url, and attempt to make it independent of all third-party libraries. I'll then try to pull in as much of that as possible into Calypso and Gutenberg, instead of duplicating functionality.

Closing this PR for now. 👍