From 4f0903cfb6169e3e8116987eacee56cbfe426317 Mon Sep 17 00:00:00 2001 From: Paul Sherman Date: Sun, 30 Oct 2016 23:27:24 -0500 Subject: [PATCH 1/3] Resolve relative paths The resolving of relative paths is done using a modified version of RFC 1808. This is done for two reasons: 1) React Router only considers pathnames, so protocol relative paths can be disregarded. 2) RFC 1808 resolves paths on the last segment with a trailing forward slash (foo/bar resolves from foo and bar is discarded), but in React Router the last segment is preserved unless it is an empty string (foo/bar and foo/bar/ resolve from bar). and components have their "to" prop automatically resolved using their parent match's pathname. If there is no parent match's pathname, then they are resolving using the root path (an empty string). For imperative navigation, the resolveLocation method is exported. The user is able to generate an absolute location by giving it a location (a pathname string or a descriptor object) and the base pathname to resolve off of. --- modules/Link.js | 24 ++++- modules/LocationUtils.js | 108 +++++++++++++++++++++++ modules/Redirect.js | 9 +- modules/__tests__/HashRouter-test.js | 1 + modules/__tests__/Link-test.js | 48 ++++++++++ modules/__tests__/LocationUtils-test.js | 111 +++++++++++++++++++++++- modules/__tests__/integration-test.js | 41 +++++++++ modules/index.js | 3 + 8 files changed, 339 insertions(+), 6 deletions(-) diff --git a/modules/Link.js b/modules/Link.js index e47f7cf096..704f28ad8c 100644 --- a/modules/Link.js +++ b/modules/Link.js @@ -1,4 +1,5 @@ import React, { PropTypes } from 'react' +import { resolveLocation } from './LocationUtils' class Link extends React.Component { static defaultProps = { @@ -56,8 +57,8 @@ class Link extends React.Component { event.preventDefault() const { router } = this.context - const { to, replace } = this.props - + const { replace } = this.props + const to = this.absoluteLocation() if (replace) { router.replaceWith(to) } else { @@ -66,8 +67,21 @@ class Link extends React.Component { } } + absoluteLocation = () => { + const { router } = this.context + let { to } = this.props + let base + // use the context.router's match if it exists + if (router.match) { + const matchState = router.match.getState() + base = matchState && matchState.match.pathname + } + return resolveLocation(to, base) + } + getIsActive() { - const { to, isActive } = this.props + const { isActive } = this.props + const to = this.absoluteLocation() return isActive( this.context.router.getState().location, @@ -79,7 +93,7 @@ class Link extends React.Component { render() { const { isActive } = this.state const { - to, + to: undefTo, // eslint-disable-line style, activeStyle, className, activeClassName, activeOnlyWhenExact, // eslint-disable-line @@ -88,6 +102,8 @@ class Link extends React.Component { ...rest } = this.props + const to = this.absoluteLocation() + return ( { ) }) } + +export const resolveLocation = (location, base) => { + if (!isRelative(location)) { + return location + } + + if (typeof location === 'string') { + return resolve(location, base) + } else { + location.pathname = resolve(location.pathname, base) + return location + } +} + +const isRelative = (location) => { + if (typeof location === 'string') { + return location.charAt(0) !== '/' + } else { + const { pathname, query, search, hash } = location + // If there is no pathname, the location should still be + // considered relative if it has a query, search, or hash + // (but not an empty query) + if (!pathname) { + return (query && Object.keys(query).length) || !!search || !!hash + } + return pathname.charAt(0) !== '/' + } +} + +// works similarly, but not exactly the same as RFC 1808 +// https://tools.ietf.org/html/rfc1808#section-4 +// base is the base URL and path is the URL to resolve. +// this differentiates from the RFC because it treats +// the url pattern "foo/bar" the same as "foo/bar/" where the +// relative path will be joined after "bar" +const resolve = (path, base = '') => { + if (path === undefined) { + return base + } else if (base === '') { + return '/' + path + } + + // RFC 1808 drops the last base segment (step 6) and joins off + // of its parent. This does not happen in here because the last + // segment of the base because we want to join off of the last + // segment. If the base ends in a forward slash, strip it so that + // we join off of the segment before that instead. + if (base[base.length-1] === '/') { + base = base.slice(0,-1) + } + + const baseSegments = splitSegments(base) + const { pathname, extra } = removeExtra(path) + // don't need to resolve if there is no pathname + if (pathname === '') { + return base + extra + } + + // filter out all ./ segments (step 6.a) + const relativeSegments = splitSegments(pathname).filter(s => s !== '.') + return joinSegments([...baseSegments, ...relativeSegments]) + extra +} + +// remove any '//' from the path because this doesn't mean anything +// and will interfere in replacement +const splitSegments = (path) => path.replace('//','/').split('/') + +// if the original location was a string (not a descriptor), then the search +// or hash can still be attached to the path, so remove it before resolving +const removeExtra = (path) => { + const hashIndex = path.indexOf('#') + const searchIndex = path.indexOf('?') + let splitIndex + if (searchIndex >= 0 && hashIndex >= 0) { + splitIndex = Math.min(searchIndex, hashIndex) + } else if (searchIndex >=0) { + splitIndex = searchIndex + } else if (hashIndex >= 0) { + splitIndex = hashIndex + } + + return { + pathname: splitIndex !== undefined ? path.slice(0, splitIndex) : path, + extra: splitIndex !== undefined ? path.slice(splitIndex) : '' + } +} + +const joinSegments = (segments) => { + // (step 6.c) + return segments + .reduce((acc, segment) => { + if (segment !== '..') { + return acc.concat(segment) + } + // remove /.. but not ../.. (or /.., but the only empty + // string segment should be the root segment). To do this, + // we want to verify that the last kept item is not a '..' or a ''. + // If it is a '..', that means that we already failed to match + // (['', 'foo', '..'] will never occur) + const last = acc[acc.length-1] + if (last !== '..' && last !== '') { + return acc.slice(0, -1) + } else { + return acc.concat('..') + } + }, []) + .join('/') +} diff --git a/modules/Redirect.js b/modules/Redirect.js index 2d74a1d728..afbf244a7a 100644 --- a/modules/Redirect.js +++ b/modules/Redirect.js @@ -1,4 +1,5 @@ import React, { PropTypes } from 'react' +import { resolveLocation } from './LocationUtils' class Redirect extends React.Component { static defaultProps = { @@ -34,7 +35,13 @@ class Redirect extends React.Component { redirect() { const { router } = this.context - const { to, push } = this.props + let { to } = this.props + const { push } = this.props + + const routerState = router && router.getState() + const matchState = routerState && routerState.match + const base = matchState && matchState.pathname + to = resolveLocation(to, base) const navigate = push ? router.transitionTo : router.replaceWith navigate(to) } diff --git a/modules/__tests__/HashRouter-test.js b/modules/__tests__/HashRouter-test.js index 1630d6ed00..a38c5cf5b6 100644 --- a/modules/__tests__/HashRouter-test.js +++ b/modules/__tests__/HashRouter-test.js @@ -8,6 +8,7 @@ describe('HashRouter', () => { const div = document.createElement('div') afterEach(() => { + history.replaceState(null, document.title, '#') unmountComponentAtNode(div) }) diff --git a/modules/__tests__/Link-test.js b/modules/__tests__/Link-test.js index 33733d84ac..f7e4959227 100644 --- a/modules/__tests__/Link-test.js +++ b/modules/__tests__/Link-test.js @@ -4,6 +4,7 @@ import Link from '../Link' import MemoryRouter from '../MemoryRouter' import StaticRouter from '../StaticRouter' import { render, unmountComponentAtNode } from 'react-dom' +import Match from '../Match' import { Simulate } from 'react-addons-test-utils' const { click } = Simulate @@ -29,6 +30,40 @@ describe('Link', () => { expect(div.querySelector('a').getAttribute('href')).toEqual('/foo') }) + describe('relative path', () => { + it('works with search/hash pathnames', () => { + const pathname = 'test?this/../isfine' + const div = document.createElement('div') + render(, div) + expect(div.querySelector('a').getAttribute('href')).toEqual(`/${pathname}`) + }) + + describe('with context.match', () => { + const BASE = '/a/b' + const LinkInMatch = ({pattern = BASE, ...props}) => ( + + ( + + )} /> + + ) + + it('resolves using parent pathname', () => { + const div = document.createElement('div') + render(, div) + expect(div.querySelector('a').getAttribute('href')).toEqual('/a/b/foo') + }) + }) + + describe('without context.match', () => { + it('resolves using root', () => { + const div = document.createElement('div') + render(, div) + expect(div.querySelector('a').getAttribute('href')).toEqual('/foo') + }) + }) + }) + describe('with context.router', () => { it('uses router.createHref to build the href', () => { const CONTEXT_HREF = 'CONTEXT_HREF' @@ -223,6 +258,19 @@ describe('Link', () => { const a = div.querySelector('a') expect(a.className).toEqual('active') }) + + it('works with relative links', () => { + const div = document.createElement('div') + render(( + + ), div) + const a = div.querySelector('a') + expect(a.className).toEqual('active') + }) }) }) diff --git a/modules/__tests__/LocationUtils-test.js b/modules/__tests__/LocationUtils-test.js index 4f1390f499..74ec75cd0e 100644 --- a/modules/__tests__/LocationUtils-test.js +++ b/modules/__tests__/LocationUtils-test.js @@ -1,6 +1,10 @@ import expect from 'expect' import { parse, stringify } from 'query-string' -import { createRouterLocation, createRouterPath } from '../LocationUtils' +import { + createRouterLocation, + createRouterPath, + resolveLocation +} from '../LocationUtils' describe('LocationUtils', () => { describe('createRouterLocation', () => { @@ -195,4 +199,109 @@ describe('LocationUtils', () => { }) }) }) + + describe('resolveLocation', () => { + const BASE = '/a/b' + + describe('string location', () => { + it('returns path if absolute', () => { + const path = '/foo' + expect(resolveLocation(path)).toBe(path) + }) + }) + + describe('object location', () => { + it('does not affect the pathname if absolute', () => { + const descriptor = { pathname: '/foo' } + expect(resolveLocation(descriptor).pathname).toBe(descriptor.pathname) + }) + + describe('no pathname', () => { + it('resolves to base if there is a query', () => { + const descriptor = { + query: {a: 'b'} + } + expect(resolveLocation(descriptor, BASE).pathname).toBe(BASE) + }) + + it('ignores empty query when determining if relative', () => { + const descriptor = { + query: {} + } + expect(resolveLocation(descriptor, BASE).pathname).toBe(undefined) + }) + + it('resolves to base if there is a query', () => { + const descriptor = { + search: '?a=b' + } + expect(resolveLocation(descriptor, BASE).pathname).toBe(BASE) + }) + + it('resolves to base if there is a hash', () => { + const descriptor = { + hash: '#foo' + } + expect(resolveLocation(descriptor, BASE).pathname).toBe(BASE) + }) + }) + }) + + it('removes unnecessary segments', () => { + const input = 'foo//../bar' + expect(resolveLocation(input, BASE)).toEqual('/a/b/bar') + }) + + describe('rfc1808', () => { + + // https://tools.ietf.org/html/rfc1808#section-5.1 + it('passes normal examples', () => { + const cases = [ + { input: 'g', output: '/a/b/g' }, + { input: './g', output: '/a/b/g' }, + { input: 'g/', output: '/a/b/g/' }, + { input: '/g', output: '/g' }, + { input: '?y', output: '/a/b?y' }, + { input: 'g?y', output: '/a/b/g?y' }, + { input: 'g?y/./x', output: '/a/b/g?y/./x' }, + { input: '#s', output: '/a/b#s' }, + { input: 'g#s', output: '/a/b/g#s' }, + { input: 'g#s/./x', output: '/a/b/g#s/./x' }, + { input: 'g?y#s', output: '/a/b/g?y#s' }, + { input: '.', output: '/a/b' }, + { input: './', output: '/a/b/' }, + { input: '..', output: '/a' }, + { input: '../', output: '/a/' }, + { input: '../g', output: '/a/g' }, + { input: '../..', output: '' }, + { input: '../../', output: '/' }, + { input: '../../g', output: '/g' } + ] + cases.forEach(test => { + expect(resolveLocation(test.input, BASE)).toBe(test.output) + }) + }) + + // https://tools.ietf.org/html/rfc1808#section-5.2 + it('passes abnormal examples', () => { + const cases = [ + { input: '../../g', output: '/g' }, + { input: '../../../g', output: '/../g' }, + { input: '/./g', output: '/./g' }, + { input: '/../g', output: '/../g' }, + { input: 'g.', output: '/a/b/g.' }, + { input: '.g', output: '/a/b/.g' }, + { input: 'g..', output: '/a/b/g..' }, + { input: '..g', output: '/a/b/..g' }, + { input: './../g', output: '/a/g' }, + { input: './g/.', output: '/a/b/g' }, + { input: 'g/./h', output: '/a/b/g/h' }, + { input: 'g/../h', output: '/a/b/h' } + ] + cases.forEach(test => { + expect(resolveLocation(test.input, BASE)).toBe(test.output) + }) + }) + }) + }) }) diff --git a/modules/__tests__/integration-test.js b/modules/__tests__/integration-test.js index d2b501179b..8fbef071fd 100644 --- a/modules/__tests__/integration-test.js +++ b/modules/__tests__/integration-test.js @@ -484,3 +484,44 @@ describe('Integration Tests', () => { }) }) }) + +describe('Link with relative to', () => { + it('navigates', () => { + const leftClickEvent = { + defaultPrevented: false, + preventDefault() { this.defaultPrevented = true }, + metaKey: null, + altKey: null, + ctrlKey: null, + shiftKey: null, + button: 0 + } + const div = document.createElement('div') + const TEXT1 = 'I AM PAGE 1' + const TEXT2 = 'I AM PAGE 2' + render(( + +
+ One + ( +
+

{TEXT1}

+ Two + ( +

{TEXT2}

+ )}/> +
+ )}/> +
+ + ), div) + expect(div.innerHTML).toNotContain(TEXT1) + + Simulate.click(div.querySelector('#one'), leftClickEvent) + expect(div.innerHTML).toContain(TEXT1) + expect(div.innerHTML).toNotContain(TEXT2) + + Simulate.click(div.querySelector('#two'), leftClickEvent) + expect(div.innerHTML).toContain(TEXT2) + }) +}) diff --git a/modules/index.js b/modules/index.js index 26dad307a1..28e816491d 100644 --- a/modules/index.js +++ b/modules/index.js @@ -17,3 +17,6 @@ export StaticRouter from './StaticRouter' // Util for creating who-knows-what! export matchPattern from './matchPattern' + +// Util for resolving relative paths +export { resolveLocation } from './LocationUtils' From 4ac3b73d1c2bea4c70b2e34d65b8fb5ea9ed4f30 Mon Sep 17 00:00:00 2001 From: Paul Sherman Date: Tue, 22 Nov 2016 10:26:31 -0600 Subject: [PATCH 2/3] fix how to get Redirect base --- modules/Redirect.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/modules/Redirect.js b/modules/Redirect.js index afbf244a7a..b54cce7277 100644 --- a/modules/Redirect.js +++ b/modules/Redirect.js @@ -37,10 +37,11 @@ class Redirect extends React.Component { const { router } = this.context let { to } = this.props const { push } = this.props - - const routerState = router && router.getState() - const matchState = routerState && routerState.match - const base = matchState && matchState.pathname + let base + if (router.match) { + const matchState = router.match.getState() + base = matchState && matchState.match.pathname + } to = resolveLocation(to, base) const navigate = push ? router.transitionTo : router.replaceWith navigate(to) From 32cfdf11fa0577aaf3229d3a54b0397fe5159474 Mon Sep 17 00:00:00 2001 From: Paul Sherman Date: Tue, 6 Dec 2016 14:12:04 -0600 Subject: [PATCH 3/3] for loop ~3x faster --- modules/LocationUtils.js | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/modules/LocationUtils.js b/modules/LocationUtils.js index 5042e11388..02f62586f6 100644 --- a/modules/LocationUtils.js +++ b/modules/LocationUtils.js @@ -121,22 +121,25 @@ const removeExtra = (path) => { const joinSegments = (segments) => { // (step 6.c) - return segments - .reduce((acc, segment) => { - if (segment !== '..') { - return acc.concat(segment) - } - // remove /.. but not ../.. (or /.., but the only empty - // string segment should be the root segment). To do this, - // we want to verify that the last kept item is not a '..' or a ''. - // If it is a '..', that means that we already failed to match - // (['', 'foo', '..'] will never occur) - const last = acc[acc.length-1] - if (last !== '..' && last !== '') { - return acc.slice(0, -1) - } else { - return acc.concat('..') - } - }, []) - .join('/') + const output = [] + const length = segments.length + for (let i=0; i/.. but not ../.. (or /.., but the only empty + // string segment should be the root segment). To do this, + // we want to verify that the last kept item is not a '..' or a ''. + // If it is a '..', that means that we already failed to match + // (['', 'foo', '..'] will never occur in the output array) + const last = output[output.length-1] + if (last !== '..' && last !== '') { + output.pop() + } else { + output.push('..') + } + } + return output.join('/') }