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.

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

Add better hash URL support. #1250

Merged
merged 5 commits into from
Feb 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 10 additions & 8 deletions client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import HeadManager from './head-manager'
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__: {
Expand All @@ -15,14 +15,15 @@ const {
err,
pathname,
query
}
},
location
} = window

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
Expand All @@ -34,11 +35,12 @@ const container = document.getElementById('__next')
export default (onError) => {
const emitter = new EventEmitter()

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
}
Expand All @@ -57,7 +59,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) {
Expand All @@ -73,7 +75,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

Expand Down
22 changes: 20 additions & 2 deletions lib/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div>
<Container {...containerProps} />
Expand All @@ -29,6 +29,24 @@ export default class App extends Component {
}

class Container extends Component {
componentDidMount () {
this.scrollToHash()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to do the scrolling here because, in the router there's no way we could know when the correct DOM is available.

}

componentDidUpdate () {
this.scrollToHash()
}

scrollToHash () {
const { hash } = this.props
const el = document.getElementById(hash)
if (el) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about <a name="some-hash">?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I didn't know it's supported. Let's add it.
PRs welcome always :)

// If we call scrollIntoView() in here without a setTimeout
// it won't scroll properly.
setTimeout(() => el.scrollIntoView(), 0)
}
}

shouldComponentUpdate (nextProps) {
// need this check not to rerender component which has already thrown an error
return !shallowEquals(this.props, nextProps)
Expand Down
78 changes: 36 additions & 42 deletions lib/router/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,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 './'
import fetch from 'unfetch'

Expand All @@ -26,7 +26,7 @@ if (typeof window !== 'undefined' && typeof navigator.serviceWorker !== 'undefin
}

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)
Expand All @@ -41,6 +41,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)
Expand All @@ -66,42 +67,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
}

const { url, as } = e.state
const { pathname, query } = parse(url, true)

this.abortComponentLoad(as)

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) {
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) {
Expand Down Expand Up @@ -160,6 +131,13 @@ export default class Router extends EventEmitter {
this.abortComponentLoad(as)
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('replaceState', 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
Expand All @@ -180,9 +158,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)
Expand Down Expand Up @@ -228,12 +207,33 @@ 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, newHash ] = as.split('#')

// 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) {
return this.pathname !== pathname || !shallowEquals(query, this.query)
}
Expand Down Expand Up @@ -346,12 +346,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(/\/$/, '') || '/'
}
Expand Down
6 changes: 6 additions & 0 deletions lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
28 changes: 28 additions & 0 deletions test/integration/basic/pages/nav/hash-changes.js
Original file line number Diff line number Diff line change
@@ -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 (
<div id='hash-changes-page'>
<Link href='#via-link'>
<a id='via-link'>Via Link</a>
</Link>
<a href='#via-a' id='via-a'>Via A</a>
<Link href='/nav/hash-changes'>
<a id='page-url'>Page URL</a>
</Link>
<p>COUNT: {this.props.count}</p>
</div>
)
}
}
60 changes: 60 additions & 0 deletions test/integration/basic/test/client-navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,5 +119,65 @@ export default (context, render) => {
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()
})
})
})
})
}
3 changes: 2 additions & 1 deletion test/integration/basic/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ describe('Basic Features', () => {
renderViaHTTP(context.appPort, '/nav'),
renderViaHTTP(context.appPort, '/nav/about'),
renderViaHTTP(context.appPort, '/nav/querystring'),
renderViaHTTP(context.appPort, '/nav/self-reload')
renderViaHTTP(context.appPort, '/nav/self-reload'),
renderViaHTTP(context.appPort, '/nav/hash-changes')
])
})
afterAll(() => stopApp(context.server))
Expand Down