Skip to content

Commit

Permalink
should not change method to replaceState unless asPath is the same (#…
Browse files Browse the repository at this point in the history
…6033)

original code in `/lib/router/router.js`
```
  urlIsNew (pathname, query) {
    return this.pathname !== pathname || !shallowEquals(query, this.query)
  }
```
the urlIsNew compare `this.pathname` to an argument `pathname`
the invokers:
```
    // If asked to change the current URL we should reload the current page
    // (not location.reload() but reload getInitialProps and other Next.js stuffs)
    // We also need to set the method = replaceState always
    // as this should not go into the history (That's how browsers work)
    if (!this.urlIsNew(asPathname, asQuery)) {
      method = 'replaceState'
    }
```
the parameter here is `asPathname` destructured from `asPath`

so here is a problem when we reuse a single page rendered in two asPaths

pages/a.js
```
<>
  <Link href='/a'><a>goto a</a></Link>
  <Link href='/a' as='/b'><a>goto b</a></Link>
</>
```
If we navigate to page /a, then click 'goto b', actually the history is replaced, not pushed.
It is expected that history could be correctly pushed and popped as long as the browser url is changed.
  • Loading branch information
tangye1234 authored and timneutkens committed Jan 11, 2019
1 parent f7477a9 commit ab46c45
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 5 deletions.
10 changes: 6 additions & 4 deletions lib/router/router.js
Expand Up @@ -164,14 +164,14 @@ export default class Router {
return true
}

const { pathname: asPathname, query: asQuery } = parse(as, true)
const { pathname, query } = parse(url, true)

// If asked to change the current URL we should reload the current page
// (not location.reload() but reload getInitialProps and other Next.js stuffs)
// We also need to set the method = replaceState always
// as this should not go into the history (That's how browsers work)
if (!this.urlIsNew(asPathname, asQuery)) {
// We should compare the new asPath to the current asPath, not the url
if (!this.urlIsNew(as)) {
method = 'replaceState'
}

Expand Down Expand Up @@ -339,8 +339,10 @@ export default class Router {
}
}

urlIsNew (pathname, query) {
return this.pathname !== pathname || !shallowEquals(query, this.query)
urlIsNew (asPath) {
const { pathname, query } = parse(asPath, true)
const { pathname: curPathname } = parse(this.asPath, true)
return curPathname !== pathname || !shallowEquals(query, this.query)
}

isShallowRoutingPossible (route) {
Expand Down
11 changes: 10 additions & 1 deletion test/integration/basic/pages/nav/as-path-pushstate.js
Expand Up @@ -9,7 +9,16 @@ export default withRouter(({router: {asPath, query}}) => {
<a id='hello'>hello</a>
</Link>
</div>

<div>
<Link href='/nav/as-path-pushstate' as='/something/else'>
<a id='else'>else</a>
</Link>
</div>
<div>
<Link href='/nav/as-path-pushstate' as='/nav/as-path-pushstate'>
<a id='hello2'>normal hello</a>
</Link>
</div>
{query.something === 'hello' && <Link href='/nav/as-path-pushstate?something=hello' as='/something/same-query'>
<a id='same-query'>same query</a>
</Link>}
Expand Down
5 changes: 5 additions & 0 deletions test/integration/basic/test/client-navigation.js
Expand Up @@ -648,6 +648,11 @@ export default (context, render) => {
await browser.back().waitForElementByCss('#something-hello')
const queryThree = JSON.parse(await browser.elementByCss('#router-query').text())
expect(queryThree.something).toBe('hello')
await browser.elementByCss('#else').click().waitForElementByCss('#something-else')
await browser.elementByCss('#hello2').click().waitForElementByCss('#nav-as-path-pushstate')
await browser.back().waitForElementByCss('#something-else')
const queryFour = JSON.parse(await browser.elementByCss('#router-query').text())
expect(queryFour.something).toBe(undefined)
} finally {
if (browser) {
browser.close()
Expand Down

0 comments on commit ab46c45

Please sign in to comment.