Packages: Stop using Node.js built-ins in browser-packaged code

[aduth]

We should seek to move away from referencing Node.js built-ins from modules, e.g. import { parse } from 'url';, where 'url' is a reference to a browser-compatible variant of the Node url module.

Reasons include:

• These automated polyfills are slated for removal in future versions of Webpack (reference)
• There is redundancy with other module usage such as the use of qs in the @wordpress/url package
  • Contributes to larger overall bundle sizes
• The polyfills are not implemented using ES Modules and thus require pulling in much more code than is actually used (example, see full list)

Instances:

• gutenberg/packages/block-library/src/embed/embed-preview.js

  Line 10 in 837fda7

  import { parse } from 'url';

• gutenberg/packages/editor/src/editor-styles/transforms/url-rewrite.js

  Line 4 in 837fda7

  import { parse, resolve } from 'url';

(Questionable: Instances within @wordpress/e2e-tests and @wordpress/e2e-test-utils ? These may be compiled with Webpack, despite not being intended for use in a browser)

With this, it should be considered to disable automatic polyfills from Webpack (reference).

[aduth]

As discovered in the course of discussing Automattic/wp-calypso#37606 , since Node 10.0.0 there is a URL global which (to my knowledge) adheres to the expected behavior of the browser global URL. Thus, for the 'url' built-in specifically, we should aim to refactor using this global.

[sgomes]

As discussed in Automattic/wp-calypso#37606, I will be attempting to rewrite @wordpress/url using only URL/URLSearchParams and no third-party libraries. In the process of doing so, I will likely need to add new methods to it that may help with the transition away from the deprecated url.

[chrisvanpatten]

@sgomes It's worth noting specifically for addQueryArgs that qs was chosen because it supports query strings with PHP-style arrays. (See #8300, #7339, and #7086.)

Given the following query string object:

const paramObj = {
  query: [ 'first', 'second' ],
}

In order to read the array, PHP will expect it to be serialized as:

const queryString = qs.stringify( paramObj )
console.log( queryString )

// query[0]=first&query[1]=second

However, URLSearchParams will serialize it as:

const queryString = new URLSearchParams( paramObj )
console.log( queryString.toString() )

// query=first%2Csecond

That's not to say it can't be done, just that extra care will need to be taken.

[aduth]

@chrisvanpatten Good to mention. Though at least based on the tests, I think the expectation is query[0]=first&query[1]=second ?

gutenberg/packages/url/src/test/index.test.js

Lines 342 to 347 in 6fc0b70

	it( 'should update args to an URL with array parameters', () => {
	const url = 'https://andalouses.example/beach?time[]=10&time[]=11';
	const args = { beach: [ 'sand', 'rock' ] };
	expect( safeDecodeURI( addQueryArgs( url, args ) ) ).toBe( 'https://andalouses.example/beach?time[0]=10&time[1]=11&beach[0]=sand&beach[1]=rock' );
	} );

That we have unit tests for this should at least help serve as a base against which an implementation can target.

[chrisvanpatten]

@aduth Technically PHP resolves it with or without the explicit key (it assumes a numerically indexed array if no keys are provided), although you are right qs does include the key by default. I'll update my example!

EDIT: Looking again at the tests, one additional case that is supported, but we don't have a test for, is named keys, e.g.

const obj = {
  query: { 'first': 'value', 'second': 'value' }
};

console.log( qs.stringify( obj ) );

// query[first]=value&query[second]=value

URLSearchParams chokes a bit on this one, and serializes it as query=%5Bobject+Object%5D.

[aduth]

To this point, for an implementation, the distinction would likely come in using URLSearchParams#append instead of URLSearchParams#set:

if the same key is appended multiple times it will appear in the parameter string multiple times for each value.

Unfortunately it doesn't handle the addition of the [] square brackets automatically.

[sgomes]

Thanks, everyone! I'll take this into account. It seems like a PHP idiom, as I believe the more common way of doing things is simply repeating the param name, with no square brackets, e.g. http://example.com/?foo=bar&foo=baz.

Is there a good source where I can read up on the exact PHP syntax we want to enable? It's trivial to implement the empty square brackets, but since we're talking optional indices too, I want to make sure I'm covering the whole thing.

qs is extremely complex with a bunch of overkill functionality for our use-case (I believe), like supporting nested objects up to 5 levels deep and all kinds of parsing options. I'd rather avoid having all that complexity and sticking to the minimum set of functionality we want to enable, so it would be good to have a clear idea of what that is, exactly.

[sgomes]

EDIT: Looking again at the tests, one additional case that is supported, but we don't have a test for, is named keys, e.g.

const obj = {
  query: { 'first': 'value', 'second': 'value' }
};

console.log( qs.stringify( obj ) );

// query[first]=value&query[second]=value

URLSearchParams chokes a bit on this one, and serializes it as query=%5Bobject+Object%5D.

This is definitely supported by qs, yup. Is it something we want in @wordpress/url, though? It's possible to replicate, but qs has such a large API surface area that I'd like to see if we could rein it in a little and only implement features that are needed and currently in use, even if that means a major version bump.

[sgomes]

@aduth @chrisvanpatten

I found the manual page for http_build_query, which seems to match both the array and object functionality that was being discussed.

Is the goal to ensure that @wordpress/url can handle all of the same syntax as http_build_query? I can definitely work on that, but I just want to make sure in the interest of reducing scope.

[chrisvanpatten]

I get that from a purely bundle size/complexity perspective supporting the entire API is not ideal, but addQueryArg is used in a bunch of places, and it’s especially important in things like the ServerSideRender component, where the block attributes are passed as args on a GET request. Given the various complex shapes that block attributes can take, I think any implementation needs to strive for as full parity as possible, or it risks breaking functionality for existing users.

Personally, I feel like this is a case where qs, with its scope and extensive proven usage, provides enough advantages that outweigh the benefits of a custom implementation. But that’s just me :)

[sgomes]

Thanks, @chrisvanpatten! I suppose this is a case where it's hard to move away from qs, since that's what was used from the beginning, and so much has been built on top of it. It's hard to move away from a complex API with an excess of features, since you'd effectively have to audit all the code that has been written, to determine which features are actually being used. I wish libraries like qs were built with a restricted set of features by default, and advanced features explicitly requested through options 😔

Given the situation, I'll drop my effort in making @wordpress/url independent of qs, as well as the effort in migrating Calypso to use @wordpress/url's addQueryArgs instead of its own. Calypso doesn't require the additional complexity of array and object parameters, so I'd rather not introduce it, lest it end up in the same situation where it's unable to migrate away from qs in the future.

[aduth]

When adopting URL, we should take care to provide a polyfill for environments where it is not supported (namely, IE11).

There is prior art for how this has been done in core:

• https://github.com/WordPress/wordpress-develop/blob/29f0d74/src/wp-includes/script-loader.php#L117-L129
• https://github.com/WordPress/wordpress-develop/blob/29f0d74/tools/webpack/packages.js#L71-L75

Until it can be patched in core, I expect we can wp_add_inline_script to the existing wp-polyfill, using a test for 'URL' in window presence via wp_get_script_polyfill.

Since we're already using polyfill-library for a Node#contains polyfill, this may be a sufficient option for us:

https://github.com/Financial-Times/polyfill-library/blob/master/polyfills/URL/polyfill.js

[aduth]

This is largely resolved by #19823. As of those changes, there are no longer any occurrences of the Node URL module.

There is also #20693 awaiting review which would further consolidate logic by dropping the qs library.

As a follow-up task, I've created #21647 as a minimal step to change Webpack configuration to forbid its reintroduction. As best I can tell, Webpack is following through on the promise to remove Node polyfills in the next major version.