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.

Sign up for GitHub

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

Jump to bottom

Fixed pathname check in router #10547

Merged
merged 6 commits into from Feb 19, 2020
Merged

Fixed pathname check in router #10547

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4be2b2d
Fixed pathname check in router
dijs Feb 16, 2020
e104aef
Updated change and added trial test
dijs Feb 19, 2020
670d0d5
Merge remote-tracking branch 'upstream/canary' into patch-1
ijjk Feb 19, 2020
5065037
Update test
ijjk Feb 19, 2020
b1c12e4
Merge branch 'canary' into patch-1
ijjk Feb 19, 2020
e167f25
Merge branch 'canary' into patch-1
ijjk Feb 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/next/next-server/lib/router/router.ts
View file
Open in desktop
Expand Up @@ -235,7 +235,8 @@ export default class Router implements BaseRouter {
if (
e.state &&
this.isSsr &&
e.state.url === this.pathname &&
// Remove the query portion of the URL when checking paths
e.state.url.replace(/\?(.)+/, '') === this.pathname &&
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
e.state.url.replace(/\?(.)+/, '') === this.pathname &&
parse(e.state.url).pathname === this.pathname &&

Let's parse it and grab the pathname to make sure we're getting the pathname correctly and let's also move the e.state.as === this.asPath check above the pathname one so that if the asPath mismatches we avoid parsing the pathname unnecessarily

Can you also add a test in test/integration/production/index.test.js similar to the should navigate to external site and back test in that file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the code changes, but I am having a really hard time getting the test enviornment to work on my system. I installed chromedriver but the tests are failing with your version only supports v81... Also, every time I run yarn testonly it completely crushes my machine with all the node processes. I added a test, but I am sure the expectations need to be updated.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good, went ahead and updated the test the rest of the way. Thanks for the PR!

e.state.as === this.asPath
) {
return
Expand Down