Skip to content
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

Add some new url helper functions #10885

Merged
merged 4 commits into from
Oct 23, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/url/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
## 2.2.0 (Unreleased)

### Features

- Added `getQueryArg`.
- Added `hasQueryArg`.
- Added `removeQueryArgs`.

## 2.1.0 (2018-10-16)

### Features
Expand Down
9 changes: 9 additions & 0 deletions packages/url/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example is wrong; needs second parameter.


// Checks whether a URL contains a given query arg.
const hasBar = hasQueryArg( 'https://wordpress.org?foo=bar&bar=baz', 'bar' ); // true

// Removes one or more query args from the given URL.
const newUrl = removeQueryArgs( 'https://wordpress.org?foo=bar&bar=baz&baz=foobar', 'foo', 'bar' ); // https://wordpress.org?baz=foobar
```

<br/><br/><p align="center"><img src="https://s.w.org/style/images/codeispoetry.png?1" alt="Code is Poetry." /></p>
45 changes: 45 additions & 0 deletions packages/url/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,51 @@ export function addQueryArgs( url, args ) {
return baseUrl + '?' + stringify( { ...query, ...args } );
}

/**
* Returns a single query argument of the url
*
* @param {string} url URL
* @param {string} arg Query arg name
*
* @return {Array|string} Query arg value.
*/
export function getQueryArg( url, arg ) {
const queryStringIndex = url.indexOf( '?' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const query = queryStringIndex !== -1 ? parse( url.substr( queryStringIndex + 1 ) ) : {};

return query[ arg ];
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @param {...string} arg Query arg name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used as a spread parameter.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* Removes arguments from the query string of the url
*
* @param {string} url URL
* @param {string} args Query Args
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used as a spread parameter.

*
* @return {string} Updated URL
*/
export function removeQueryArgs( url, ...args ) {
const queryStringIndex = url.indexOf( '?' );
const query = queryStringIndex !== -1 ? parse( url.substr( queryStringIndex + 1 ) ) : {};
const baseUrl = queryStringIndex !== -1 ? url.substr( 0, queryStringIndex ) : url;

args.forEach( ( arg ) => delete query[ arg ] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return baseUrl + '?' + stringify( query );
}

/**
* Prepends "http://" to a url, if it looks like something that is meant to be a TLD.
*
Expand Down
57 changes: 57 additions & 0 deletions packages/url/src/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import { every } from 'lodash';
import {
isURL,
addQueryArgs,
getQueryArg,
hasQueryArg,
removeQueryArgs,
prependHTTP,
safeDecodeURI,
} from '../';
Expand Down Expand Up @@ -69,6 +72,60 @@ describe( 'addQueryArgs', () => {
} );
} );

describe( 'getQueryArg', () => {
it( 'should get the value of an existing query arg', () => {
const url = 'https://andalouses.example/beach?foo=bar&bar=baz';

expect( getQueryArg( url, 'foo' ) ).toBe( 'bar' );
} );

it( 'should not return a value of an unknown query arg', () => {
const url = 'https://andalouses.example/beach?foo=bar&bar=baz';

expect( getQueryArg( url, 'baz' ) ).toBeUndefined();
} );

it( 'should get the value of an arry query arg', () => {
const url = 'https://andalouses.example/beach?foo[]=bar&foo[]=baz';

expect( getQueryArg( url, 'foo' ) ).toEqual( [ 'bar', 'baz' ] );
} );
} );

describe( 'hasQueryArg', () => {
it( 'should return true for an existing query arg', () => {
const url = 'https://andalouses.example/beach?foo=bar&bar=baz';

expect( hasQueryArg( url, 'foo' ) ).toBeTruthy();
} );

it( 'should return false for an unknown query arg', () => {
const url = 'https://andalouses.example/beach?foo=bar&bar=baz';

expect( hasQueryArg( url, 'baz' ) ).toBeFalsy();
} );

it( 'should return true for an arry query arg', () => {
const url = 'https://andalouses.example/beach?foo[]=bar&foo[]=baz';

expect( hasQueryArg( url, 'foo' ) ).toBeTruthy();
} );
} );

describe( 'removeQueryArgs', () => {
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
it( 'should not change URL not containing query args', () => {
const url = 'https://andalouses.example/beach?foo=bar&bar=baz';

expect( removeQueryArgs( url, 'baz', 'test' ) ).toEqual( url );
} );

it( 'should remove existing query args', () => {
const url = 'https://andalouses.example/beach?foo=bar&baz=foo&bar=baz';

expect( removeQueryArgs( url, 'foo', 'bar' ) ).toEqual( 'https://andalouses.example/beach?baz=foo' );
} );
} );

describe( 'prependHTTP', () => {
it( 'should prepend http to a domain', () => {
const url = 'wordpress.org';
Expand Down