Skip to content
Merged
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
Prev Previous commit
Next Next commit
Parse the query string rather than using a regex replace
  • Loading branch information
nylen committed May 5, 2017
commit 53fb259257aeb790157af82f39172301ee6c5a99
14 changes: 8 additions & 6 deletions editor/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { get } from 'lodash';
import { parse, stringify } from 'querystring';

export function savePost( dispatch, post ) {
const isNew = ! post.id;
Expand All @@ -17,12 +18,13 @@ export function savePost( dispatch, post ) {
isNew,
} );
if ( isNew && window.history.replaceState ) {
window.history.replaceState(
{},
'Post ' + newPost.id,
window.location.href.replace( /&post_id=[^&]*$/, '' )
+ '&post_id=' + newPost.id
);
const urlPieces = window.location.href.split( '?' );
const qs = parse( urlPieces[ 1 ] || '' );
const newUrl = urlPieces[ 0 ] + '?' + stringify( {
...qs,
Copy link
Member

Choose a reason for hiding this comment

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

Minor: The object spread syntax creates another clone step; this is the equivalent of:

Object.assign( {}, qs, { post_id: newPost.id } );

Since we've created and have no plans to reuse the object outside the scope of the function, I think it'd be fine enough to mutate qs by assigning directly into it:

Object.assign( qs, { post_id: newPost.id } );

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this, and I think the code clarity / elegance due to using the spread operator is worth the extra clone step. I don't think the performance is a concern here as this is not called very often and the browser's replaceState is going to be orders of magnitude more expensive than the assign.

post_id: newPost.id,
} );
window.history.replaceState( {}, 'Post ' + newPost.id, newUrl );
}
} ).fail( ( err ) => {
dispatch( {
Expand Down