From 7750bf0987cd1b6e32cd1c0cc034fa463381f5a0 Mon Sep 17 00:00:00 2001 From: Arunoda Susiripala Date: Thu, 23 Feb 2017 01:45:43 +0530 Subject: [PATCH 1/4] Add better hash URL support. 1. Add scrolling to given id related to hash 2. Hash changes won't trigger getInitialProps --- client/index.js | 14 +++++++----- lib/app.js | 20 ++++++++++++++-- lib/router/router.js | 54 ++++++++++++++++---------------------------- 3 files changed, 45 insertions(+), 43 deletions(-) diff --git a/client/index.js b/client/index.js index a6a9f2206a28..52d926f40abf 100644 --- a/client/index.js +++ b/client/index.js @@ -17,7 +17,8 @@ const { err, pathname, query - } + }, + location } = window const Component = evalScript(component).default @@ -37,11 +38,12 @@ export default (onError) => { const emitter = new EventEmitter() if (ids && ids.length) rehydrate(ids) - router.subscribe(({ Component, props, err }) => { - render({ Component, props, err, emitter }, onError) + router.subscribe(({ Component, props, hash, err }) => { + render({ Component, props, err, hash, emitter }, onError) }) - render({ Component, props, err, emitter }, onError) + const hash = location.hash.substring(1) + render({ Component, props, hash, err, emitter }, onError) return emitter } @@ -60,7 +62,7 @@ async function renderErrorComponent (err) { await doRender({ Component: ErrorComponent, props, err }) } -async function doRender ({ Component, props, err, emitter }) { +async function doRender ({ Component, props, hash, err, emitter }) { if (!props && Component && Component !== ErrorComponent && lastAppProps.Component === ErrorComponent) { @@ -76,7 +78,7 @@ async function doRender ({ Component, props, err, emitter }) { Component = Component || lastAppProps.Component props = props || lastAppProps.props - const appProps = { Component, props, err, router, headManager } + const appProps = { Component, props, hash, err, router, headManager } // lastAppProps has to be set before ReactDom.render to account for ReactDom throwing an error. lastAppProps = appProps diff --git a/lib/app.js b/lib/app.js index 979ed7bd42a5..5e7c49c87b44 100644 --- a/lib/app.js +++ b/lib/app.js @@ -17,8 +17,8 @@ export default class App extends Component { } render () { - const { Component, props, err, router } = this.props - const containerProps = { Component, props, router } + const { Component, props, hash, err, router } = this.props + const containerProps = { Component, props, hash, router } return
@@ -29,6 +29,22 @@ export default class App extends Component { } class Container extends Component { + componentDidMount () { + this.scrollToHash() + } + + componentDidUpdate () { + this.scrollToHash() + } + + scrollToHash () { + const { hash } = this.props + const el = document.getElementById(hash) + if (el) { + setTimeout(() => el.scrollIntoView(), 0) + } + } + shouldComponentUpdate (nextProps) { // need this check not to rerender component which has already thrown an error return !shallowEquals(this.props, nextProps) diff --git a/lib/router/router.js b/lib/router/router.js index e3141a359770..5c08f3951452 100644 --- a/lib/router/router.js +++ b/lib/router/router.js @@ -54,43 +54,12 @@ export default class Router extends EventEmitter { // Actually, for (1) we don't need to nothing. But it's hard to detect that event. // So, doing the following for (1) does no harm. const { pathname, query } = this - this.replace(format({ pathname, query }), getURL()) + this.changeState('replaceState', format({ pathname, query }), getURL()) return } - this.abortComponentLoad() - const { url, as } = e.state - const { pathname, query } = parse(url, true) - - if (!this.urlIsNew(pathname, query)) { - this.emit('routeChangeStart', as) - this.emit('routeChangeComplete', as) - return - } - - const route = toRoute(pathname) - - this.emit('routeChangeStart', as) - const { - data, - props, - error - } = await this.getRouteInfo(route, pathname, query, as) - - if (error && error.cancelled) { - this.emit('routeChangeError', error, as) - return - } - - this.route = route - this.set(pathname, query, { ...data, props }) - - if (error) { - this.emit('routeChangeError', error, as) - } else { - this.emit('routeChangeComplete', as) - } + this.replace(url, as) } update (route, Component) { @@ -150,6 +119,11 @@ export default class Router extends EventEmitter { this.abortComponentLoad() const { pathname, query } = parse(url, true) + if (this.onlyAHashChange(as)) { + this.changeState(method, url, as) + return + } + // If asked to change the current URL we should reload the current page // (not location.reload() but reload getInitalProps and other Next.js stuffs) // We also need to set the method = replaceState always @@ -171,9 +145,10 @@ export default class Router extends EventEmitter { } this.changeState(method, url, as) + const hash = window.location.hash.substring(1) this.route = route - this.set(pathname, query, { ...data, props }) + this.set(pathname, query, as, { ...data, props, hash }) if (error) { this.emit('routeChangeError', error, as) @@ -219,12 +194,21 @@ export default class Router extends EventEmitter { return routeInfo } - set (pathname, query, data) { + set (pathname, query, as, data) { this.pathname = pathname this.query = query + this.as = as this.notify(data) } + onlyAHashChange (as) { + if (!this.as) return false + const [ oldUrlNoHash ] = this.as.split('#') + const [ newUrlNoHash ] = as.split('#') + + return oldUrlNoHash === newUrlNoHash + } + urlIsNew (pathname, query) { return this.pathname !== pathname || !shallowEquals(query, this.query) } From 715b4de57cadbff67ddee9e884f43ace74640360 Mon Sep 17 00:00:00 2001 From: Arunoda Susiripala Date: Thu, 23 Feb 2017 02:07:55 +0530 Subject: [PATCH 2/4] Add some comments. --- lib/app.js | 2 ++ lib/router/router.js | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/app.js b/lib/app.js index 5e7c49c87b44..5ff939653c4a 100644 --- a/lib/app.js +++ b/lib/app.js @@ -41,6 +41,8 @@ class Container extends Component { const { hash } = this.props const el = document.getElementById(hash) if (el) { + // If we call scrollIntoView() in here without a setTimeout + // it won't scroll properly. setTimeout(() => el.scrollIntoView(), 0) } } diff --git a/lib/router/router.js b/lib/router/router.js index 5c08f3951452..c310ae7e4bcf 100644 --- a/lib/router/router.js +++ b/lib/router/router.js @@ -119,8 +119,10 @@ export default class Router extends EventEmitter { this.abortComponentLoad() const { pathname, query } = parse(url, true) + // If the url change is only related to a hash change + // We should not proceed. We should only replace the state. if (this.onlyAHashChange(as)) { - this.changeState(method, url, as) + this.changeState('replaceState', url, as) return } From e2ca30d2130f0b00ef1db61efba87b0dc4eb7c79 Mon Sep 17 00:00:00 2001 From: Arunoda Susiripala Date: Thu, 23 Feb 2017 02:22:10 +0530 Subject: [PATCH 3/4] Fix tests. --- lib/router/router.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/router/router.js b/lib/router/router.js index c310ae7e4bcf..38be1e219be8 100644 --- a/lib/router/router.js +++ b/lib/router/router.js @@ -206,9 +206,21 @@ export default class Router extends EventEmitter { onlyAHashChange (as) { if (!this.as) return false const [ oldUrlNoHash ] = this.as.split('#') - const [ newUrlNoHash ] = as.split('#') + const [ newUrlNoHash, newHash ] = as.split('#') - return oldUrlNoHash === newUrlNoHash + // If the urls are change, there's more than a hash change + if (oldUrlNoHash !== newUrlNoHash) { + return false + } + + // If there's no hash in the new url, we can't consider it as a hash change + if (!newHash) { + return false + } + + // Now there's a hash in the new URL. + // We don't need to worry about the old hash. + return true } urlIsNew (pathname, query) { From 6444878076d50d860105c289a2ae423387c7b643 Mon Sep 17 00:00:00 2001 From: Arunoda Susiripala Date: Thu, 23 Feb 2017 02:54:28 +0530 Subject: [PATCH 4/4] Add some test cases. --- client/index.js | 4 +- lib/router/router.js | 11 +--- lib/utils.js | 6 ++ .../basic/pages/nav/hash-changes.js | 28 +++++++++ .../basic/test/client-navigation.js | 60 +++++++++++++++++++ 5 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 test/integration/basic/pages/nav/hash-changes.js diff --git a/client/index.js b/client/index.js index 52d926f40abf..940c65b89b2d 100644 --- a/client/index.js +++ b/client/index.js @@ -6,7 +6,7 @@ import { rehydrate } from '../lib/css' import { createRouter } from '../lib/router' import App from '../lib/app' import evalScript from '../lib/eval-script' -import { loadGetInitialProps } from '../lib/utils' +import { loadGetInitialProps, getURL } from '../lib/utils' const { __NEXT_DATA__: { @@ -25,7 +25,7 @@ const Component = evalScript(component).default const ErrorComponent = evalScript(errorComponent).default let lastAppProps -export const router = createRouter(pathname, query, { +export const router = createRouter(pathname, query, getURL(), { Component, ErrorComponent, err diff --git a/lib/router/router.js b/lib/router/router.js index 38be1e219be8..4c2473d7c293 100644 --- a/lib/router/router.js +++ b/lib/router/router.js @@ -5,7 +5,7 @@ import { EventEmitter } from 'events' import evalScript from '../eval-script' import shallowEquals from '../shallow-equals' import PQueue from '../p-queue' -import { loadGetInitialProps, getLocationOrigin } from '../utils' +import { loadGetInitialProps, getURL } from '../utils' import { _notifyBuildIdMismatch } from './' // Add "fetch" polyfill for older browsers @@ -14,7 +14,7 @@ if (typeof window !== 'undefined') { } export default class Router extends EventEmitter { - constructor (pathname, query, { Component, ErrorComponent, err } = {}) { + constructor (pathname, query, as, { Component, ErrorComponent, err } = {}) { super() // represents the current component key this.route = toRoute(pathname) @@ -29,6 +29,7 @@ export default class Router extends EventEmitter { this.ErrorComponent = ErrorComponent this.pathname = pathname this.query = query + this.as = as this.subscriptions = new Set() this.componentLoadCancel = null this.onPopState = this.onPopState.bind(this) @@ -330,12 +331,6 @@ export default class Router extends EventEmitter { } } -function getURL () { - const { href } = window.location - const origin = getLocationOrigin() - return href.substring(origin.length) -} - function toRoute (path) { return path.replace(/\/$/, '') || '/' } diff --git a/lib/utils.js b/lib/utils.js index d5ba6c5f301b..2831c0a465ff 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -58,3 +58,9 @@ export function getLocationOrigin () { const { protocol, hostname, port } = window.location return `${protocol}//${hostname}${port ? ':' + port : ''}` } + +export function getURL () { + const { href } = window.location + const origin = getLocationOrigin() + return href.substring(origin.length) +} diff --git a/test/integration/basic/pages/nav/hash-changes.js b/test/integration/basic/pages/nav/hash-changes.js new file mode 100644 index 000000000000..99699796c5a3 --- /dev/null +++ b/test/integration/basic/pages/nav/hash-changes.js @@ -0,0 +1,28 @@ +import React, { Component } from 'react' +import Link from 'next/link' + +let count = 0 + +export default class SelfReload extends Component { + static getInitialProps ({ res }) { + if (res) return { count: 0 } + count += 1 + + return { count } + } + + render () { + return ( +
+ + Via Link + + Via A + + Page URL + +

COUNT: {this.props.count}

+
+ ) + } +} diff --git a/test/integration/basic/test/client-navigation.js b/test/integration/basic/test/client-navigation.js index 5f856fbb9e86..55dc11d3a25f 100644 --- a/test/integration/basic/test/client-navigation.js +++ b/test/integration/basic/test/client-navigation.js @@ -119,5 +119,65 @@ export default (context) => { await browser.close() }) }) + + describe('with hash changes', () => { + describe('when hash change via Link', () => { + it('should not run getInitialProps', async () => { + const browser = await webdriver(context.appPort, '/nav/hash-changes') + + const counter = await browser + .elementByCss('#via-link').click() + .elementByCss('p').text() + + expect(counter).toBe('COUNT: 0') + + await browser.close() + }) + }) + + describe('when hash change via A tag', () => { + it('should not run getInitialProps', async () => { + const browser = await webdriver(context.appPort, '/nav/hash-changes') + + const counter = await browser + .elementByCss('#via-a').click() + .elementByCss('p').text() + + expect(counter).toBe('COUNT: 0') + + await browser.close() + }) + }) + + describe('when hash get removed', () => { + it('should not run getInitialProps', async () => { + const browser = await webdriver(context.appPort, '/nav/hash-changes') + + const counter = await browser + .elementByCss('#via-a').click() + .elementByCss('#page-url').click() + .elementByCss('p').text() + + expect(counter).toBe('COUNT: 1') + + await browser.close() + }) + }) + + describe('when hash changed to a different hash', () => { + it('should not run getInitialProps', async () => { + const browser = await webdriver(context.appPort, '/nav/hash-changes') + + const counter = await browser + .elementByCss('#via-a').click() + .elementByCss('#via-link').click() + .elementByCss('p').text() + + expect(counter).toBe('COUNT: 0') + + await browser.close() + }) + }) + }) }) }