Add some new url helper functions #10885

swissspidy · 2018-10-21T22:03:23Z

Description

As discussed in #10862 and outlined in #10879, some more URL modification helpers might make sense. I figured when I add getQueryArg and hasQueryArg I might just as well complete the group with removeQueryArgs.

How has this been tested?

I included a few unit tests for each of the new functions.

Screenshots

Types of changes

Extends @wordpress/url package with new functions getQueryArg, hasQueryArg, and removeQueryArgs,

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

Fixes #10879.

See #10879.

gziolo · 2018-10-22T11:51:15Z

Are there any places in the existing codebase which could benefit from using those new methods?

Can we document those newly introduced methods in README file?

swissspidy · 2018-10-22T12:39:16Z

@gziolo Good input, updated the readme accordingly!

hasQueryArg()
Could certainly be used in Add user locale api-fetch middleware #10862.
Generally useful little helper for developers.
getQueryArg()
PHP equivalent proposed for WordPress core, so here we'd have some parity.
Generally useful little helper for developers.
removeQueryArgs()
PHP equivalent exists in WordPress core, so here we'd have some parity.
Generally useful little helper for developers, for example when someone wants to remove the query arg added by Add user locale api-fetch middleware #10862 or modify any other URL.

Perhaps there are more potential usages in Gutenberg, but it's tough to say after a quick glance.

Looking at getStablePath() in createPreloadingMiddleware(), that one could be added to this package too. The stringify method in qs even has a sort option, so this could be leveraged as well.

export function sortQueryArgs( url ) {
  const queryStringIndex = url.indexOf( '?' );
  const query = queryStringIndex !== -1 ? parse( url.substr( queryStringIndex + 1 ) ) : {};
  const baseUrl = queryStringIndex !== -1 ? url.substr( 0, queryStringIndex ) : url;

  return baseUrl + '?' + stringify( query, { sort: (a, b) => a.localeCompare( b ) } );
}

chrisvanpatten · 2018-10-22T13:03:05Z

I wonder if getQueryArg and hasQueryArg could be used in the pagination middleware to detect per_page=-1? I think right now it's based on a string match, which is fine, but this is definitely nicer.

swissspidy · 2018-10-22T13:23:41Z

Indeed requestContainsUnboundedQuery() could use getQueryArg() for sure:

const requestContainsUnboundedQuery = ( options ) => {
	const pathIsUnbounded = options.path && getQueryArg( options.path, 'per_page' ) === '-1';
	const urlIsUnbounded = options.url && getQueryArg( options.url, 'per_page' ) === '-1';

	return pathIsUnbounded || urlIsUnbounded;
};

It's actually safer than just using indexOf( 'per_page=-1' ), because that also matches my_custom_items_per_page=-1, which is what I'd consider a bug.

gziolo · 2018-10-22T13:46:56Z

Let's make sure we open follow-up issue when this one lands. It should be labeled with Good first issue :)

aduth · 2018-10-22T14:01:58Z

packages/url/src/index.js

+ * @return {boolean} Whether or not the URL contains the query aeg.
+ */
+export function hasQueryArg( url, arg ) {
+	return typeof getQueryArg( url, arg ) !== 'undefined';


FYI: Comparing to undefined directly should work just as well, and saves some characters. typeof would be useful only if trying to reference a variable which hasn't been initialized, or if we were targeting pre-ES5 browsers (IE7) where undefined could be overwritten.

aduth · 2018-10-22T14:03:50Z

packages/url/src/index.js

+ * @return {Array|string} Query arg value.
+ */
+export function getQueryArg( url, arg ) {
+	const queryStringIndex = url.indexOf( '?' );


Given repetition in the file, seems we could do for an internal utility function(s) which return the parsed query and/or return the query fragment from a string.

Yeah it's not really nice at the moment. I think we should address this in a following PR.

aduth · 2018-10-22T14:07:23Z

packages/url/src/index.js

+ * @return {Array|string} Query arg value.
+ */
+export function getQueryArg( url, arg ) {
+	const queryStringIndex = url.indexOf( '?' );


Or just use a proper URL parsers, like require( 'url' ).parse( url ).search†

https://nodejs.org/docs/latest/api/url.html#url_url_search

† I assume reason we haven't is due to size of the imported (stub) module.

aduth · 2018-10-22T14:07:36Z

packages/url/src/index.js

+ * Determines whether the URL contains a given query arg.
+ *
+ * @param  {string}    url URL
+ * @param  {...string} arg Query arg name


This is not used as a spread parameter.

aduth · 2018-10-22T14:08:17Z

packages/url/src/index.js

+/**
+ * Determines whether the URL contains a given query arg.
+ *
+ * @param  {string}    url URL


Per standards examples, we've avoided bothering to align the @param type with @return.

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/#functions

aduth · 2018-10-22T14:08:51Z

packages/url/src/index.js

+	const query = queryStringIndex !== -1 ? parse( url.substr( queryStringIndex + 1 ) ) : {};
+	const baseUrl = queryStringIndex !== -1 ? url.substr( 0, queryStringIndex ) : url;
+
+	args.forEach( ( arg ) => delete query[ arg ] );


Guess we've not brought in Lodash yet as a dependency of this one, but good use-case for omit.

https://lodash.com/docs/4.17.10#omit

Yeah I looked into this one but since it's a one-liner it shouldn't be necessary to add a dependency just for this use case.

aduth · 2018-10-22T14:09:17Z

packages/url/src/index.js

+ * Removes arguments from the query string of the url
+ *
+ * @param  {string} url  URL
+ * @param  {string} args Query Args


This is used as a spread parameter.

aduth · 2018-10-22T14:09:49Z

packages/url/README.md

@@ -25,6 +25,15 @@ const newURL = addQueryArgs( 'https://google.com', { q: 'test' } ); // https://

 // Prepends 'http://' to URLs that are probably mean to have them
 const actualURL = prependHTTP( 'wordpress.org' ); // http://wordpress.org
+
+// Gets a single query arg from the given URL.
+const foo = getQueryArg( 'https://wordpress.org?foo=bar&bar=baz' ); // bar


Example is wrong; needs second parameter.

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

aduth

Nice utilities 👍

Meta-note: As best I can tell, this closes #10879, but we've not included magic keywords to automate the closure.

swissspidy · 2018-10-22T16:17:35Z

See -> Fixes in PR description for auto closing 🙂

aduth · 2018-10-22T16:18:06Z

Oh, I missed it there at the bottom 👍

aduth · 2018-10-22T16:18:24Z

Oh, you changed it! 🤦‍♂️

gziolo · 2018-10-23T06:13:23Z

See -> Fixes in PR description for auto closing 🙂

It worked out 👍 Thanks for addressing my feedback 💯

…rnmobile/fix-validation-errorrs-after-block-splitting * 'master' of https://github.com/WordPress/gutenberg: Add some new url helper functions (#10885) Components: Avoid SlotFill props destructuring (#10921) Update Package Changelogs (#10887) chore(release): publish Update plugin version to 4.1 RC2. (#10893)

See #10885.

* Add user locale api-fetch middleware See #10811 * Don't modify existing _locale query arg * Add changelog entry * Update changelog * Use new hasQueryArg helper function See #10885. * Match on simply the route path, instead of malformed query param * Fully mock embedding test * Skip the embedding test until we can make the travis env mock the requests * Skip demo test too, for now

* Add some new url helper functions See WordPress#10879. * Document new functions in readme * Improve docs * Add another test for removeQueryArgs()

* Add user locale api-fetch middleware See WordPress#10811 * Don't modify existing _locale query arg * Add changelog entry * Update changelog * Use new hasQueryArg helper function See WordPress#10885. * Match on simply the route path, instead of malformed query param * Fully mock embedding test * Skip the embedding test until we can make the travis env mock the requests * Skip demo test too, for now

Add some new url helper functions

f136bfb

See #10879.

gziolo added the [Package] Url /packages/url label Oct 22, 2018

Document new functions in readme

0f34a26

gziolo requested a review from a team October 22, 2018 13:45

aduth reviewed Oct 22, 2018

View reviewed changes

swissspidy added 2 commits October 22, 2018 16:45

Improve docs

150d94f

Add another test for removeQueryArgs()

8ea88ba

aduth approved these changes Oct 22, 2018

View reviewed changes

gziolo added this to the 4.2 milestone Oct 23, 2018

gziolo merged commit 898f2f7 into master Oct 23, 2018

gziolo deleted the add/url-helpers branch October 23, 2018 06:12

swissspidy added a commit that referenced this pull request Oct 23, 2018

Use new hasQueryArg helper function

9b9f5ea

See #10885.

swissspidy mentioned this pull request Oct 23, 2018

Add user locale api-fetch middleware #10862

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some new url helper functions #10885

Add some new url helper functions #10885

swissspidy commented Oct 21, 2018 •

edited by gziolo

Loading

gziolo commented Oct 22, 2018 •

edited

Loading

swissspidy commented Oct 22, 2018

chrisvanpatten commented Oct 22, 2018

swissspidy commented Oct 22, 2018

gziolo commented Oct 22, 2018

aduth Oct 22, 2018

aduth Oct 22, 2018

swissspidy Oct 22, 2018

aduth Oct 22, 2018

aduth Oct 22, 2018

aduth Oct 22, 2018

aduth Oct 22, 2018

swissspidy Oct 22, 2018

aduth Oct 22, 2018

aduth Oct 22, 2018

aduth left a comment

swissspidy commented Oct 22, 2018

aduth commented Oct 22, 2018

aduth commented Oct 22, 2018

gziolo commented Oct 23, 2018 •

edited

Loading

Add some new url helper functions #10885

Add some new url helper functions #10885

Conversation

swissspidy commented Oct 21, 2018 • edited by gziolo Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

gziolo commented Oct 22, 2018 • edited Loading

swissspidy commented Oct 22, 2018

chrisvanpatten commented Oct 22, 2018

swissspidy commented Oct 22, 2018

gziolo commented Oct 22, 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

swissspidy commented Oct 22, 2018

aduth commented Oct 22, 2018

aduth commented Oct 22, 2018

gziolo commented Oct 23, 2018 • edited Loading

swissspidy commented Oct 21, 2018 •

edited by gziolo

Loading

gziolo commented Oct 22, 2018 •

edited

Loading

gziolo commented Oct 23, 2018 •

edited

Loading