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.

Sign up for GitHub

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

Move getStablePath function into the wordrpess/url package #35992

Merged
merged 5 commits into from
Nov 1, 2021
Merged

Move getStablePath function into the wordrpess/url package #35992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a325f8
Commit WIP.
anton-vlasenko Oct 26, 2021
2d7ffe5
Refactor preloading.js to use the normalizePath function from the wor…
anton-vlasenko Oct 27, 2021
c50fe73
Refactor index.js to use the normalizePath function from the wordpres…
anton-vlasenko Oct 27, 2021
c53aafc
Move the normalizePath uinit test from wordpress/api-fetch to the wor…
anton-vlasenko Oct 27, 2021
d724344
Update changelogs.
anton-vlasenko Oct 28, 2021
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
4 changes: 4 additions & 0 deletions packages/api-fetch/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Internal
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to add changelog for internal function. They aren't expected to be used by users anyway.

Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@kevin940726
Excuse me, but I don't understand why we don't have to add such changes to the changelog.
This document states that:

For each pull request, you should always include relevant changes in a "Unreleased" heading at the top of the file. You should add the heading if it doesn't already exist.

Also, it says this:

"Internal" - Changes which do not have an impact on the public interface or behavior of the module (requires a patch version bump).

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I guess that we're just not strictly following this 😅 ? Anyway, this looks fine to me.

Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Nov 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I guess that we're just not strictly following this

Yes, it looks like so. 😅
But maybe we should (I am not excluding myself from the list).
Again, thank you for the review!


- Removed `getStablePath` function. Please use `normalizePath` from `@wordpress/url` package instead ([#35992](https://github.com/WordPress/gutenberg/pull/35992)).``

## 5.2.0 (2021-07-21)

### New feature
Expand Down
38 changes: 4 additions & 34 deletions packages/api-fetch/src/middlewares/preloading.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,53 +1,23 @@
/**
* Given a path, returns a normalized path where equal query parameter values
* will be treated as identical, regardless of order they appear in the original
* text.
*
* @param {string} path Original path.
*
* @return {string} Normalized path.
* WordPress dependencies
*/
export function getStablePath( path ) {
const splitted = path.split( '?' );
const query = splitted[ 1 ];
const base = splitted[ 0 ];
if ( ! query ) {
return base;
}

// 'b=1&c=2&a=5'
return (
base +
'?' +
query
// [ 'b=1', 'c=2', 'a=5' ]
.split( '&' )
// [ [ 'b, '1' ], [ 'c', '2' ], [ 'a', '5' ] ]
.map( ( entry ) => entry.split( '=' ) )
// [ [ 'a', '5' ], [ 'b, '1' ], [ 'c', '2' ] ]
.sort( ( a, b ) => a[ 0 ].localeCompare( b[ 0 ] ) )
// [ 'a=5', 'b=1', 'c=2' ]
.map( ( pair ) => pair.join( '=' ) )
// 'a=5&b=1&c=2'
.join( '&' )
);
}
import { normalizePath } from '@wordpress/url';

/**
* @param {Record<string, any>} preloadedData
* @return {import('../types').APIFetchMiddleware} Preloading middleware.
*/
function createPreloadingMiddleware( preloadedData ) {
const cache = Object.keys( preloadedData ).reduce( ( result, path ) => {
result[ getStablePath( path ) ] = preloadedData[ path ];
result[ normalizePath( path ) ] = preloadedData[ path ];
return result;
}, /** @type {Record<string, any>} */ ( {} ) );

return ( options, next ) => {
const { parse = true } = options;
if ( typeof options.path === 'string' ) {
const method = options.method || 'GET';
const path = getStablePath( options.path );
const path = normalizePath( options.path );

if ( 'GET' === method && cache[ path ] ) {
const cacheData = cache[ path ];
Expand Down
25 changes: 1 addition & 24 deletions packages/api-fetch/src/middlewares/test/preloading.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,9 @@
/**
* Internal dependencies
*/
import createPreloadingMiddleware, { getStablePath } from '../preloading';
import createPreloadingMiddleware from '../preloading';

describe( 'Preloading Middleware', () => {
describe( 'getStablePath', () => {
it( 'returns same value if no query parameters', () => {
const path = '/foo/bar';

expect( getStablePath( path ) ).toBe( path );
} );

it( 'returns a stable path', () => {
const abc = getStablePath( '/foo/bar?a=5&b=1&c=2' );
const bca = getStablePath( '/foo/bar?b=1&c=2&a=5' );
const bac = getStablePath( '/foo/bar?b=1&a=5&c=2' );
const acb = getStablePath( '/foo/bar?a=5&c=2&b=1' );
const cba = getStablePath( '/foo/bar?c=2&b=1&a=5' );
const cab = getStablePath( '/foo/bar?c=2&a=5&b=1' );

expect( abc ).toBe( bca );
expect( bca ).toBe( bac );
expect( bac ).toBe( acb );
expect( acb ).toBe( cba );
expect( cba ).toBe( cab );
} );
} );

describe( 'given preloaded data', () => {
describe( 'when data is requested from a preloaded endpoint', () => {
describe( 'and it is requested for the first time', () => {
Expand Down
8 changes: 7 additions & 1 deletion packages/edit-navigation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,10 @@

## Unreleased

- Initial version of the package.
### Internal

- Removed `getStablePath` function. Please use `normalizePath` from `@wordpress/url` package instead ([#35992](https://github.com/WordPress/gutenberg/pull/35992)).

## 1.0.0

- Initial version of the package.
44 changes: 7 additions & 37 deletions packages/edit-navigation/src/utils/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* WordPress dependencies
*/
import { normalizePath } from '@wordpress/url';

/**
* The purpose of this function is to create a middleware that is responsible for preloading menu-related data.
* It uses data that is returned from the /__experimental/menus endpoint for requests
Expand All @@ -10,7 +15,7 @@
*/
export function createMenuPreloadingMiddleware( preloadedData ) {
const cache = Object.keys( preloadedData ).reduce( ( result, path ) => {
result[ getStablePath( path ) ] = preloadedData[ path ];
result[ normalizePath( path ) ] = preloadedData[ path ];
return result;
}, /** @type {Record<string, any>} */ ( {} ) );

Expand All @@ -28,7 +33,7 @@ export function createMenuPreloadingMiddleware( preloadedData ) {
return next( options );
}

const path = getStablePath( options.path );
const path = normalizePath( options.path );
if ( ! menusDataLoaded && cache[ path ] ) {
menusDataLoaded = true;
return sendSuccessResponse( cache[ path ], parse );
Expand Down Expand Up @@ -85,38 +90,3 @@ function sendSuccessResponse( responseData, parse ) {
} )
);
}

/**
* Given a path, returns a normalized path where equal query parameter values
* will be treated as identical, regardless of order they appear in the original
* text.
*
* @param {string} path Original path.
*
* @return {string} Normalized path.
*/
export function getStablePath( path ) {
const splitted = path.split( '?' );
const query = splitted[ 1 ];
const base = splitted[ 0 ];
if ( ! query ) {
return base;
}

// 'b=1&c=2&a=5'
return (
base +
'?' +
query
// [ 'b=1', 'c=2', 'a=5' ]
.split( '&' )
// [ [ 'b, '1' ], [ 'c', '2' ], [ 'a', '5' ] ]
.map( ( entry ) => entry.split( '=' ) )
// [ [ 'a', '5' ], [ 'b, '1' ], [ 'c', '2' ] ]
.sort( ( a, b ) => a[ 0 ].localeCompare( b[ 0 ] ) )
// [ 'a=5', 'b=1', 'c=2' ]
.map( ( pair ) => pair.join( '=' ) )
// 'a=5&b=1&c=2'
.join( '&' )
);
}
4 changes: 4 additions & 0 deletions packages/url/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### New Feature

- Added new `normalizePath` function ([#35992](https://github.com/WordPress/gutenberg/pull/35992)).

## 3.2.3 (2021-10-12)

### Bug Fix
Expand Down
14 changes: 14 additions & 0 deletions packages/url/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,20 @@ _Returns_

- `boolean`: True if the argument contains a valid query string.

### normalizePath

Given a path, returns a normalized path where equal query parameter values
will be treated as identical, regardless of order they appear in the original
text.

_Parameters_

- _path_ `string`: Original path.

_Returns_

- `string`: Normalized path.

### prependHTTP

Prepends "http\://" to a url, if it looks like something that is meant to be a TLD.
Expand Down
1 change: 1 addition & 0 deletions packages/url/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ export { safeDecodeURIComponent } from './safe-decode-uri-component';
export { filterURLForDisplay } from './filter-url-for-display';
export { cleanForSlug } from './clean-for-slug';
export { getFilename } from './get-filename';
export { normalizePath } from './normalize-path';
34 changes: 34 additions & 0 deletions packages/url/src/normalize-path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Given a path, returns a normalized path where equal query parameter values
* will be treated as identical, regardless of order they appear in the original
* text.
*
* @param {string} path Original path.
*
* @return {string} Normalized path.
*/
export function normalizePath( path ) {
const splitted = path.split( '?' );
const query = splitted[ 1 ];
const base = splitted[ 0 ];
if ( ! query ) {
return base;
}

// 'b=1&c=2&a=5'
return (
base +
'?' +
query
// [ 'b=1', 'c=2', 'a=5' ]
.split( '&' )
// [ [ 'b, '1' ], [ 'c', '2' ], [ 'a', '5' ] ]
.map( ( entry ) => entry.split( '=' ) )
// [ [ 'a', '5' ], [ 'b, '1' ], [ 'c', '2' ] ]
.sort( ( a, b ) => a[ 0 ].localeCompare( b[ 0 ] ) )
// [ 'a=5', 'b=1', 'c=2' ]
.map( ( pair ) => pair.join( '=' ) )
// 'a=5&b=1&c=2'
.join( '&' )
);
Comment on lines +18 to +33
Copy link
Member

Choose a reason for hiding this comment

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

Now that we don't have to support IE, we can do this instead:

const searchParams = new URLSearchParams( query );
searchParams.sort();

return `${base}?${ searchParams.toString() }`;

Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Oct 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good idea. Thank you, @kevin940726 !
I've updated the changelog files in d724344
UPD:

Left a potential improvement we could do, but not necessary for this PR.

I've created an issue for this improvement: #36046

}
24 changes: 24 additions & 0 deletions packages/url/src/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
cleanForSlug,
getQueryArgs,
getFilename,
normalizePath,
} from '../';
import wptData from './fixtures/wpt-data';

Expand Down Expand Up @@ -985,3 +986,26 @@ describe( 'cleanForSlug', () => {
expect( cleanForSlug( null ) ).toBe( '' );
} );
} );

describe( 'normalizePath', () => {
it( 'returns same value if no query parameters', () => {
const path = '/foo/bar';

expect( normalizePath( path ) ).toBe( path );
} );

it( 'returns a stable path', () => {
const abc = normalizePath( '/foo/bar?a=5&b=1&c=2' );
const bca = normalizePath( '/foo/bar?b=1&c=2&a=5' );
const bac = normalizePath( '/foo/bar?b=1&a=5&c=2' );
const acb = normalizePath( '/foo/bar?a=5&c=2&b=1' );
const cba = normalizePath( '/foo/bar?c=2&b=1&a=5' );
const cab = normalizePath( '/foo/bar?c=2&a=5&b=1' );

expect( abc ).toBe( bca );
expect( bca ).toBe( bac );
expect( bac ).toBe( acb );
expect( acb ).toBe( cba );
expect( cba ).toBe( cab );
} );
} );