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

Jump to bottom

Remove location pathname decoding #656

Merged
merged 6 commits into from
Mar 26, 2019
Merged

Remove location pathname decoding #656

Show file tree
Hide file tree
Changes from all 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
15 changes: 0 additions & 15 deletions modules/LocationUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,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 Down
16 changes: 2 additions & 14 deletions modules/__tests__/BrowserHistory-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,11 @@ describe('a browser history', () => {
});

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,15 +87,9 @@ 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);
TestSequences.LocationPathnameAlwaysSame(history, done);
});
});

Expand Down
16 changes: 2 additions & 14 deletions modules/__tests__/HashHistory-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,11 @@ describe('a hash history', () => {
});

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,15 +90,9 @@ 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);
TestSequences.LocationPathnameAlwaysSame(history, done);
});
});

Expand Down
18 changes: 0 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 @@ -131,24 +131,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
16 changes: 2 additions & 14 deletions modules/__tests__/MemoryHistory-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,11 @@ describe('a memory history', () => {
});

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,15 +81,9 @@ 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);
TestSequences.LocationPathnameAlwaysSame(history, done);
});
});

Expand Down
4 changes: 2 additions & 2 deletions ...equences/LocationPathnameAlwaysDecoded.js → ...stSequences/LocationPathnameAlwaysSame.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default function(history, done) {
},
location => {
expect(location).toMatchObject({
pathname: '/歴史'
pathname: '/%E6%AD%B4%E5%8F%B2'
});

// encoded object
Expand All @@ -20,7 +20,7 @@ export default function(history, done) {
},
location => {
expect(location).toMatchObject({
pathname: '/歴史'
pathname: '/%E6%AD%B4%E5%8F%B2'
});
// unencoded string
const pathname = '/歴史';
Expand Down
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.

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

This file was deleted.

6 changes: 2 additions & 4 deletions modules/__tests__/TestSequences/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,10 @@ export { default as InitialLocationNoKey } from './InitialLocationNoKey';
export { default as InitialLocationHasKey } from './InitialLocationHasKey';
export { default as Listen } from './Listen';
export {
default as LocationPathnameAlwaysDecoded
} from './LocationPathnameAlwaysDecoded';
default as LocationPathnameAlwaysSame
} from './LocationPathnameAlwaysSame';
export { default as NoslashHashPathCoding } from './NoslashHashPathCoding';
export { default as PushEncodedLocation } from './PushEncodedLocation';
export { default as PushInvalidPathname } from './PushInvalidPathname';
export { default as PushNewLocation } from './PushNewLocation';
export { default as PushMissingPathname } from './PushMissingPathname';
export { default as PushSamePath } from './PushSamePath';
Expand All @@ -34,7 +33,6 @@ export { default as PushState } from './PushState';
export { default as PushStateWarning } from './PushStateWarning';
export { default as PushRelativePathname } from './PushRelativePathname';
export { default as PushUnicodeLocation } from './PushUnicodeLocation';
export { default as ReplaceInvalidPathname } from './ReplaceInvalidPathname';
export { default as ReplaceNewLocation } from './ReplaceNewLocation';
export { default as ReplaceSamePath } from './ReplaceSamePath';
export { default as ReplaceState } from './ReplaceState';
Expand Down