fix(scroll): leading // no longer crashes scrollBehavior init #3679
Conversation
snoozbuster
changed the title
| @@ -138,6 +139,15 @@ module.exports = { | |||
| 'scroll to anchor on load' | |||
| ) | |||
|
|
|||
| // #2593 no crash on malformed URL | |||
| .url('http://localhost:8080//scroll-behavior') | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
but then you are looking for /scroll-behavior// (or // if base is /)
There was a problem hiding this comment.
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).
| @@ -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)) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
hey @posva - how do you want me to proceed on this? is there some issue with this fix as written, and if so can you clarify so that I can fix it? |
Fixes #2593.