Skip to content

Remove location pathname decoding #656

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

Merged
merged 6 commits into from
Mar 26, 2019
Merged

Remove location pathname decoding #656

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
11bde7a
Remove location pathname decoding
OliverJAsh Jan 10, 2019
4a8985b
Update tests
OliverJAsh Jan 23, 2019
561a688
Always fully encode pathname
pshrmn Mar 24, 2019
8081f7a
Don't change pathname, but warn if not fully encoded
pshrmn Mar 25, 2019
5aaff69
Pass lint
pshrmn Mar 25, 2019
bfdb5fb
Revert encodeurl changes
pshrmn Mar 26, 2019
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
24 changes: 12 additions & 12 deletions .size-snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
{
"esm/history.js": {
"bundled": 28364,
"minified": 12515,
"gzipped": 3683,
"bundled": 28234,
"minified": 12465,
"gzipped": 3635,
"treeshaked": {
"rollup": {
"code": 208,
"import_statements": 132
"code": 226,
"import_statements": 150
},
"webpack": {
"code": 1324
"code": 1375
}
}
},
"umd/history.js": {
"bundled": 33174,
"minified": 12021,
"gzipped": 3992
"bundled": 34665,
"minified": 12190,
"gzipped": 4109
},
"umd/history.min.js": {
"bundled": 30690,
"minified": 10155,
"gzipped": 3616
"bundled": 32227,
"minified": 10251,
"gzipped": 3675
}
}
22 changes: 7 additions & 15 deletions modules/LocationUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import resolvePathname from 'resolve-pathname';
import valueEqual from 'value-equal';
import encodeurl from 'encodeurl';
import tinywarning from 'tiny-warning';

import { parsePath } from './PathUtils';

Expand Down Expand Up @@ -32,21 +34,6 @@ export function createLocation(path, state, key, currentLocation) {
location.state = state;
}

try {
location.pathname = decodeURI(location.pathname);
Copy link

Choose a reason for hiding this comment

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

I wonder if the owners of this library would prefer a warning:

// in try-catch, no sense throwing any longer
if (location.pathname !== decodeURI(location.pathname)) {
  // warn: pathnames are no longer automatically decoded
}

} catch (e) {
if (e instanceof URIError) {
throw new URIError(
'Pathname "' +
location.pathname +
'" could not be decoded. ' +
'This is likely caused by an invalid percent-encoding.'
);
} else {
throw e;
}
}

if (key) location.key = key;

if (currentLocation) {
Expand All @@ -65,6 +52,11 @@ export function createLocation(path, state, key, currentLocation) {
location.pathname = '/';
}
}

tinywarning(
location.pathname === encodeurl(location.pathname),
"location.pathname is not fully encoded, which may result in bugs."
);

return location;
}
Expand Down
26 changes: 4 additions & 22 deletions modules/__tests__/BrowserHistory-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,18 @@ describe('a browser history', () => {
});
});

describe('push with a unicode path string', () => {
it('creates a location with decoded properties', done => {
TestSequences.PushUnicodeLocation(history, done);
describe('push with a non-encoded path string', () => {
it('logs a warning', done => {
TestSequences.WarnsForNonencodedPathname(history, done);
});
});

describe('push with an encoded path string', () => {
it('creates a location object with decoded pathname', done => {
it('creates a location object with encoded pathname', done => {
TestSequences.PushEncodedLocation(history, done);
});
});

describe('push with an invalid path string (bad percent-encoding)', () => {
it('throws an error', done => {
TestSequences.PushInvalidPathname(history, done);
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was no longer valid, since we don't throw an error now we have no decoding. (We assume the pathname provided will be correctly encoded.)

describe('replace a new path', () => {
it('calls change listeners with the new location', done => {
TestSequences.ReplaceNewLocation(history, done);
Expand All @@ -93,18 +87,6 @@ describe('a browser history', () => {
});
});

describe('replace with an invalid path string (bad percent-encoding)', () => {
it('throws an error', done => {
TestSequences.ReplaceInvalidPathname(history, done);
});
});

describe('location created by encoded and unencoded pathname', () => {
it('produces the same location.pathname', done => {
TestSequences.LocationPathnameAlwaysDecoded(history, done);
});
});

describe('location created with encoded/unencoded reserved characters', () => {
it('produces different location objects', done => {
TestSequences.EncodedReservedCharacters(history, done);
Expand Down
26 changes: 4 additions & 22 deletions modules/__tests__/HashHistory-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,24 +60,18 @@ describe('a hash history', () => {
});
});

describe('push with a unicode path string', () => {
it('creates a location with decoded properties', done => {
TestSequences.PushUnicodeLocation(history, done);
describe('push with a non-encoded path string', () => {
it('logs a warning', done => {
TestSequences.WarnsForNonencodedPathname(history, done);
});
});

describe('push with an encoded path string', () => {
it('creates a location object with decoded pathname', done => {
it('creates a location object with encoded pathname', done => {
TestSequences.PushEncodedLocation(history, done);
});
});

describe('push with an invalid path string (bad percent-encoding)', () => {
it('throws an error', done => {
TestSequences.PushInvalidPathname(history, done);
});
});

describe('replace a new path', () => {
it('calls change listeners with the new location', done => {
TestSequences.ReplaceNewLocation(history, done);
Expand All @@ -96,18 +90,6 @@ describe('a hash history', () => {
});
});

describe('replace with an invalid path string (bad percent-encoding)', () => {
it('throws an error', done => {
TestSequences.ReplaceInvalidPathname(history, done);
});
});

describe('location created by encoded and unencoded pathname', () => {
it('produces the same location.pathname', done => {
TestSequences.LocationPathnameAlwaysDecoded(history, done);
});
});

describe('location created with encoded/unencoded reserved characters', () => {
it('produces different location objects', done => {
TestSequences.EncodedReservedCharacters(history, done);
Expand Down
37 changes: 19 additions & 18 deletions modules/__tests__/LocationUtils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,25 @@ describe('createLocation', () => {
});
});
});

describe('pathname encoding', () => {
describe('with a non-encoded pathname', () => {
it('warns that it received an improperly encoded pathname', () => {
let consoleWarn = console.error; // eslint-disable-line no-console
let warningMessage;

// eslint-disable-next-line no-console
console.warn = message => {
warningMessage = message;
};

createLocation('/歴史');
expect(warningMessage).toBe('location.pathname is not fully encoded, which may result in bugs.');

console.error = consoleWarn;
});
});
});

describe('with a path with no pathname', () => {
describe('given as a string', () => {
Expand Down Expand Up @@ -131,24 +150,6 @@ describe('createLocation', () => {
});
});

describe('with a path that cannot be decoded', () => {
describe('given as a string', () => {
it('throws custom message when decodeURI throws a URIError', () => {
expect(() => {
createLocation('/test%');
}).toThrow('Pathname "/test%" could not be decoded.');
});
});

describe('given as an object', () => {
it('throws custom message when decodeURI throws a URIError', () => {
expect(() => {
createLocation({ pathname: '/test%' });
}).toThrow('Pathname "/test%" could not be decoded.');
});
});
});

describe('key', () => {
it('has a key property if a key is provided', () => {
const location = createLocation('/the/path', undefined, 'key');
Expand Down
26 changes: 4 additions & 22 deletions modules/__tests__/MemoryHistory-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,18 @@ describe('a memory history', () => {
});
});

describe('push with a unicode path string', () => {
it('creates a location with decoded properties', done => {
TestSequences.PushUnicodeLocation(history, done);
describe('push with a non-encoded path string', () => {
it('logs a warning', done => {
TestSequences.WarnsForNonencodedPathname(history, done);
});
});

describe('push with an encoded path string', () => {
it('creates a location object with decoded pathname', done => {
it('creates a location object with encoded pathname', done => {
TestSequences.PushEncodedLocation(history, done);
});
});

describe('push with an invalid path string (bad percent-encoding)', () => {
it('throws an error', done => {
TestSequences.PushInvalidPathname(history, done);
});
});

describe('replace a new path', () => {
it('calls change listeners with the new location', done => {
TestSequences.ReplaceNewLocation(history, done);
Expand All @@ -87,18 +81,6 @@ describe('a memory history', () => {
});
});

describe('replace with an invalid path string (bad percent-encoding)', () => {
it('throws an error', done => {
TestSequences.ReplaceInvalidPathname(history, done);
});
});

describe('location created by encoded and unencoded pathname', () => {
it('produces the same location.pathname', done => {
TestSequences.LocationPathnameAlwaysDecoded(history, done);
});
});

describe('location created with encoded/unencoded reserved characters', () => {
it('produces different location objects', done => {
TestSequences.EncodedReservedCharacters(history, done);
Expand Down
45 changes: 0 additions & 45 deletions modules/__tests__/TestSequences/LocationPathnameAlwaysDecoded.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion modules/__tests__/TestSequences/PushEncodedLocation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export default function(history, done) {
(location, action) => {
expect(action).toBe('PUSH');
expect(location).toMatchObject({
pathname: '/歴史',
pathname: '/%E6%AD%B4%E5%8F%B2',
search: '?%E3%82%AD%E3%83%BC=%E5%80%A4',
hash: '#%E3%83%8F%E3%83%83%E3%82%B7%E3%83%A5'
});
Expand Down
17 changes: 0 additions & 17 deletions modules/__tests__/TestSequences/PushInvalidPathname.js
View file
Open in desktop

This file was deleted.

28 changes: 0 additions & 28 deletions modules/__tests__/TestSequences/PushUnicodeLocation.js
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions modules/__tests__/TestSequences/ReplaceInvalidPathname.js
View file
Open in desktop

This file was deleted.

Loading