-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Use a unique querystring package instead of three different ones #8300
Conversation
What is different about how PHP encodes a querystring than what these other packages are offering? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theurl
built-in to format, which itself adopts the behavior of thequerystring
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.
packages/editor/package.json
Outdated
"lodash": "^4.17.10", | ||
"memize": "^1.0.5", | ||
"querystring": "^0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: querystring
is a built-in, so we never should have been defining it as a dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
The Would also likely need to rebuild the |
Yes, I suspect it will, and further suggest that it'd be a good thing for us to do so. |
9f27277
to
8b2844b
Compare
Good call, updated to use @wordpress/url
I updated the |
How nice, love this change ❤️ |
@@ -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 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queryString
isn't the most accurate name for this variable assignment any longer. Maybe just query
.
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: Why is @babel/runtime
a dependency for this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
packages/url/src/index.js
Outdated
|
||
return format( { ...parsedURL, query } ); | ||
return stringifyURL( { ...parsedURL, search: stringify( query ) } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/url/src/test/index.test.js
Outdated
@@ -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', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use test
or do we use it
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅I was not even aware it exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use it
more, so probably better to settle on this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
is default if you check Jest docs 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
bfc6073
to
9f914ab
Compare
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how it worked on Calypso :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe npm is resilient enough to be allowing the extra character? At least based on the resolved qs
in package-lock.json
.
This PR updates our code base to use a unique
querystring
package instead of three different ones. I settled onhttp-build-query
because it's the PHP compliant one.Testing instructions