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

fix(scroll): leading // no longer crashes scrollBehavior init #3679

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix(scroll): leading // no longer crashes scrollBehavior init #3679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion src/util/scroll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type Router from '../index'
import { assert } from './warn'
import { getStateKey, setStateKey } from './state-key'
import { extend } from './misc'
import { cleanPath } from './path'

const positionStore = Object.create(null)

Expand All @@ -22,7 +23,8 @@ export function setupScroll () {
// preserve existing history state as it could be overriden by the user
const stateCopy = extend({}, window.history.state)
stateCopy.key = getStateKey()
window.history.replaceState(stateCopy, '', absolutePath)
// Fix for #2593 clean path to avoid crashing entire app on paths with leading double-slash (http://example.com//foo/bar)
window.history.replaceState(stateCopy, '', cleanPath(absolutePath))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the absolutePath should not remove the protocol and path in line 22 but we need to make sure this change works well with file:// protocol as well

Copy link
Author

Choose a reason for hiding this comment

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

I think that would be ideal, but I don't 100% understand what this code is written to accomplish. there aren't any UTs on this method and I wasn't able to get the e2e tests running locally due to a chromedriver/chrome version mismatch I couldn't resolve, so I kinda need to be sure of what I'm doing in order to prevent needing to push random changes to run circleci builds

window.addEventListener('popstate', handlePopState)
return () => {
window.removeEventListener('popstate', handlePopState)
Expand Down
10 changes: 10 additions & 0 deletions test/e2e/specs/scroll-behavior.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module.exports = {
const TIMEOUT = 2000

browser

.url('http://localhost:8080/scroll-behavior/')
.waitForElementVisible('#app', 1000)
.assert.count('li a', 6)
Expand Down Expand Up @@ -138,6 +139,15 @@ module.exports = {
'scroll to anchor on load'
)

// #2593 no crash on malformed URL
.url('http://localhost:8080//scroll-behavior')
Copy link
Member

Choose a reason for hiding this comment

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

I saw your comment on the issue but entering through this URL is not correct because the /scroll-behavior/ section is the base URL. Meaning that in a real scenario, the server should not serve this URL. It just happens to serve with the dev server because it's a simple fallback server that doesn't care about the base but in a real-world scenario, the fallback happens after /scroll-behavior/, the base URL. The server should actually 404 on this URL

Copy link
Author

@snoozbuster snoozbuster Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

what if the base URL is /? our production setup is to serve our vue app on the root of the domain. I see what you're saying when the app base URL is something other than the root, but in that case shouldn't this still work?

Copy link
Member

Choose a reason for hiding this comment

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

but then you are looking for /scroll-behavior// (or // if base is /)

Copy link
Author

Choose a reason for hiding this comment

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

right - in this case the base is //. the scroll-behavior part of this URL doesn't really matter to the reproduction or the fix, IIRC there just needs to be something rendered in order to assert on it being there (since the crash generally causes nothing to be rendered at all).

.execute(function () {
location.reload(true)
})
.waitForElementVisible('#app', 1000)
.assert.count('li a', 6)
.assert.containsText('.view', 'home')

.end()
}
}