Call this.set() in hash change to save this.asPath (#2825)#5519
Call this.set() in hash change to save this.asPath (#2825)#5519michaeljonathanblack wants to merge 17 commits into
Conversation
|
|
||
| // record hash change | ||
| const hash = window.location.hash.substring(1) | ||
| this.set(route, pathname, query, as, { ...routeInfo, hash }) |
There was a problem hiding this comment.
This call here is the fix. Previously, when onlyAHashChange was called, this.asPath was undefined because it was never set inside this conditional.
| routeInfo = this.components[route] | ||
| } else { | ||
| const { shallow = false } = options | ||
|
|
There was a problem hiding this comment.
Kept this block down here to avoid accidentally calling getInitialProps for a hash change (although it should be noted that we won't call this.scrollToHash(as) when we're navigating between pages with an attached hash.
There's an open issue for that, though.
|
Could you add a test for this? |
|
Let me see what I can do! |
|
@timneutkens Added unit tests. Also added a change that should close out #5161, with accompanying unit test change. |
timneutkens
left a comment
timneutkens
left a comment
There was a problem hiding this comment.
Sorry for being a pain, but this will help in the long run, could you add integration tests for this change too, include the fix for the other issue
|
I was wondering if you'd want integration tests! I'll have to do some more digging. Initially I ran into integration test issues as it seemed like the entire suite wanted to run locally, despite my protests. |
…ps' of github.com:mherodev/next.js into bugfix/vercel#2825-changing-hash-triggers-getInitialProps
| .elementByCss('p').text() | ||
|
|
||
| expect(counter).toBe('COUNT: 1') | ||
| expect(counter).toBe('COUNT: 0') |
There was a problem hiding this comment.
This test wasn't actually testing what it described; if getInitialProps was not called, count should be zero.
As far as I can tell, the Next.js router doesn't handle regular anchor-based hash changes outside of the Link component (e.g. clicking an anchor hash doesn't update the router state, so clicking #page-url in that case WOULD trigger getInitialProps)
|
Update: I don't think this resolves #5161 if that bug is related to |
|
So I was thinking, making the router aware of hashes wouldn't be the solution either, since someone might do |
Correct, the router currently doesn't understand non-Link/Router hash navigations (that seems to be by design?) We could handle that feature in another issue/PR, if that's desirable. |
|
@timneutkens Is there outstanding work for this PR? I'd like to get this merged in so we can start targeting canary and using hash-routing without triggering getInitialProps. |
2 participants
Fixes #2825