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

should not change method to replaceState unless asPath is the same #6033

Merged
merged 1 commit into from
Jan 11, 2019

Conversation

tangye1234
Copy link
Contributor

@tangye1234 tangye1234 commented Jan 11, 2019

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.

@dav-is
Copy link
Contributor

dav-is commented Jan 11, 2019

Looks like there was a caching error in TravisCI. I cleared the cache and now the tests pass 🙌

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Great PR 🙏

@timneutkens timneutkens merged commit ab46c45 into vercel:master Jan 11, 2019
dav-is pushed a commit that referenced this pull request Jan 11, 2019
…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.
timneutkens added a commit that referenced this pull request Feb 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants