Use a unique querystring package instead of three different ones #8300

youknowriad · 2018-07-30T15:36:17Z

This PR updates our code base to use a unique querystring package instead of three different ones. I settled on http-build-query because it's the PHP compliant one.

Testing instructions

Check the term selectors
Check the latest posts block
Check the embeds
Check the page attributes panel (parent page)
Check adding links (autocompletion)

aduth · 2018-07-30T15:52:04Z

I settled on http-build-query because it's the PHP compliant one.

What is different about how PHP encodes a querystring than what these other packages are offering?

aduth

A few things:

Almost every single one of these instances would be better off using addQueryArgs from the @wordpress/url package.
On that note, addQueryArgs uses the url built-in to format, which itself adopts the behavior of the querystring built-in, so we still have inconsistency.
On that note, the url shim used by Webpack (node-url) is much larger than we probably want to be loading in the browser (source). We could consider updating this to use a querystring encoder directly as well.

aduth · 2018-07-30T15:47:53Z

packages/editor/package.json

 		"lodash": "^4.17.10",
 		"memize": "^1.0.5",
-		"querystring": "^0.2.0",


Aside: querystring is a built-in, so we never should have been defining it as a dependency.

Well, it's not black or white here because depending on the bundler used by the packages consuming this package, we should define it or not.

Hmm, an interesting point. I'm still not sure it's needed, since what's published to npm should be targeting Node (CommonJS, built-ins), even if ultimately it's consumed in a web context through a bundler.

youknowriad · 2018-07-30T16:13:02Z

What is different about how PHP encodes a querystring than what these other packages are offering?

The difference is the way arrays are passed in query strings. The ServerSideRender component had a bug because of that and now has a unit test to check this. #7339

aduth · 2018-07-30T16:15:25Z

The difference is the way arrays are passed in query strings. The ServerSideRender component had a bug because of that and now has a unit test to check this. #7339

Would be good if we could communicate this somehow. Maybe not as part of this pull request, but ideally we should consolidate most all use to use addQueryArgs, then somewhere in the file / function include an inline code comment explaining the distinction.

chrisvanpatten · 2018-07-30T16:29:52Z

The @wordpress/url addQueryArgs would need to be rewritten to use http-build-query (or an inlined version of that code). Not a big deal, but just worth noting.

Would also likely need to rebuild the parse method since I expect node's URL parser also wouldn't correctly interpret PHP-style query strings.

aduth · 2018-07-30T17:48:46Z

The @wordpress/url addQueryArgs would need to be rewritten to use http-build-query

Yes, I suspect it will, and further suggest that it'd be a good thing for us to do so.

Good call, updated to use @wordpress/url

youknowriad · 2018-07-31T08:39:26Z

I updated the @wordpress/url package. It now relies on the widely used qs package which is also php compatible. I prefered this one because we need both parsing and serializing with PHP compatible querystrings.

gziolo · 2018-07-31T10:21:28Z

How nice, love this change ❤️

aduth · 2018-07-31T12:54:01Z

packages/editor/src/components/page-attributes/parent.js

@@ -65,17 +65,17 @@ const applyWithDispatch = withDispatch( ( dispatch ) => {
 const applyWithAPIDataItems = withAPIData( ( { postType, postId } ) => {
 	const isHierarchical = get( postType, [ 'hierarchical' ], false );
 	const restBase = get( postType, [ 'rest_base' ], false );
-	const queryString = stringify( {
+	const queryString = {


queryString isn't the most accurate name for this variable assignment any longer. Maybe just query.

aduth · 2018-07-31T12:55:07Z

packages/url/package.json

@@ -19,7 +19,8 @@
 	"main": "build/index.js",
 	"module": "build-module/index.js",
 	"dependencies": {
-		"@babel/runtime": "^7.0.0-beta.52"
+		"@babel/runtime": "^7.0.0-beta.52",


Aside: Why is @babel/runtime a dependency for this package?

We put it as a dependency for all packages that have transpiled code published to npm. We didn't find a better solution so far. This is what Babel docs recommend:

NOTE - Production vs. development dependencies

In most cases, you should install @babel/plugin-transform-runtime as a development dependency (with --save-dev).

npm install --save-dev @babel/plugin-transform-runtime

and @babel/runtime as a production dependency (with --save).

npm install --save @babel/runtime

The transformation plugin is typically used only in development, but the runtime itself will be depended on by your deployed/published code. See the examples below for more details.

https://new.babeljs.io/docs/en/next/babel-plugin-transform-runtime.html#installation

aduth · 2018-07-31T12:57:07Z

packages/url/src/index.js


-	return format( { ...parsedURL, query } );
+	return stringifyURL( { ...parsedURL, search: stringify( query ) } );


I'd be motivated to simplify this function further so we can remove the dependency on url (as mentioned previously, it's quite large).

I think this should work okay?

const search = stringify( args ); const joiner = url.indexOf( '?' ) === -1 ? '?' : '&'; return url + joiner + search;

I think there's a difference between the two implementation. In this particular case, we don't overwrite already present args in the url query string, but I think I can update the implementation to avoid the url package and still override existing keys.

aduth · 2018-07-31T12:57:23Z

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

@@ -24,6 +24,13 @@ describe( 'addQueryArgs', () => {

 		expect( addQueryArgs( url, args ) ).toBe( 'https://andalouses.example/beach?night=false&sun=true&sand=false' );
 	} );
+
+	test( 'should update args to an URL with array parameters', () => {


Do we use test or do we use it ?

😅I was not even aware it exists

We use both. I'm more interested in which one we should settle on. I don't care which it is, just that it's consistent.

I think we use it more, so probably better to settle on this one?

test is default if you check Jest docs 🤷‍♂️

It should be fine now.

aduth

Looks good 👍

aduth · 2018-09-07T15:14:35Z

packages/url/package.json

@@ -19,7 +19,8 @@
 	"main": "build/index.js",
 	"module": "build-module/index.js",
 	"dependencies": {
-		"@babel/runtime": "^7.0.0-beta.52"
+		"@babel/runtime": "^7.0.0-beta.52",
+		"qs": "^6.5.2s"


See the issue on this line? 😱

We really need to do something to hold ourselves more accountable for published packages working as advertised. This line makes the packages largely unusable, or at least in my attempts to use @wordpress/components on plnkr.co

I wonder how it worked on Calypso :)

Hmm, maybe npm is resilient enough to be allowing the extra character? At least based on the resolved qs in package-lock.json.

youknowriad added the [Type] Code Quality Issues or PRs that relate to code quality label Jul 30, 2018

youknowriad self-assigned this Jul 30, 2018

youknowriad requested a review from a team July 30, 2018 15:36

aduth previously requested changes Jul 30, 2018

View reviewed changes

youknowriad force-pushed the update/querystring branch from 9f27277 to 8b2844b Compare July 31, 2018 08:36

aduth previously requested changes Jul 31, 2018

View reviewed changes

aduth approved these changes Jul 31, 2018

View reviewed changes

youknowriad added 4 commits August 1, 2018 09:30

Use a unique querystring package instead of three different ones

b97cc78

Update the URL package to be PHP compliant

e8d3fdb

Drop the url package dependency from '@wordpress/url'

03c1492

Small naming convetion tweaks

9f914ab

youknowriad force-pushed the update/querystring branch from bfc6073 to 9f914ab Compare August 1, 2018 08:30

youknowriad merged commit 0c96693 into master Aug 1, 2018

youknowriad deleted the update/querystring branch August 1, 2018 08:49

aduth mentioned this pull request Aug 1, 2018

Framework: Specify missing wp-url dependencies #8362

Merged

danielbachhuber added this to the 3.5 milestone Aug 1, 2018

youknowriad mentioned this pull request Aug 2, 2018

Move embed API call out of block and into data #6678

Merged

4 tasks

aduth reviewed Sep 7, 2018

View reviewed changes

youknowriad mentioned this pull request Sep 7, 2018

Fix qs dependency typo #9692

Merged

chrisvanpatten mentioned this pull request Nov 18, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a unique querystring package instead of three different ones #8300

Use a unique querystring package instead of three different ones #8300

youknowriad commented Jul 30, 2018

aduth commented Jul 30, 2018

aduth left a comment

aduth Jul 30, 2018

youknowriad Jul 30, 2018

aduth Jul 30, 2018 •

edited

Loading

youknowriad commented Jul 30, 2018

aduth commented Jul 30, 2018 •

edited

Loading

chrisvanpatten commented Jul 30, 2018 •

edited

Loading

aduth commented Jul 30, 2018

youknowriad commented Jul 31, 2018

gziolo commented Jul 31, 2018

aduth Jul 31, 2018

aduth Jul 31, 2018

gziolo Jul 31, 2018

aduth Jul 31, 2018

youknowriad Jul 31, 2018

aduth Jul 31, 2018

youknowriad Jul 31, 2018

aduth Jul 31, 2018

youknowriad Aug 1, 2018

gziolo Aug 1, 2018

aduth left a comment

aduth Sep 7, 2018

youknowriad Sep 7, 2018

aduth Sep 7, 2018


		return format( { ...parsedURL, query } );
		return stringifyURL( { ...parsedURL, search: stringify( query ) } );

Use a unique querystring package instead of three different ones #8300

Use a unique querystring package instead of three different ones #8300

Conversation

youknowriad commented Jul 30, 2018

aduth commented Jul 30, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

youknowriad commented Jul 30, 2018

aduth commented Jul 30, 2018 • edited Loading

chrisvanpatten commented Jul 30, 2018 • edited Loading

aduth commented Jul 30, 2018

youknowriad commented Jul 31, 2018

gziolo commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Jul 30, 2018 •

edited

Loading

aduth commented Jul 30, 2018 •

edited

Loading

chrisvanpatten commented Jul 30, 2018 •

edited

Loading