Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5k
fix(scroll): leading // no longer crashes scrollBehavior init#3679
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:dev
Are you sure you want to change the base?
Conversation
| @@ -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.
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 happensafter/scroll-behavior/, the base URL. The server should actually 404 on this URL
snoozbusterDec 15, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/)
There was a problem hiding this comment.
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//. thescroll-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe theabsolutePath should not remove the protocol and path in line 22 but we need to make sure this change works well withfile:// protocol as well
There was a problem hiding this comment.
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
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.