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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(vue-app): sanitize path with trailing slash in getLocation
(#6744)
#6745
Conversation
…pathname) in `getLocation` (#6744)
Codecov Report
@@ Coverage Diff @@
## dev #6745 +/- ##
=======================================
Coverage 64.82% 64.82%
=======================================
Files 78 78
Lines 2718 2718
Branches 706 706
=======================================
Hits 1762 1762
Misses 729 729
Partials 227 227
Continue to review full report at Codecov.
|
@pi0 fixed getLocation again, because at first I did mutating with path and it failed tests. Now I don't mutate path UPD: |
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 have nothing to add then what I've said before that we should add some sanitation methods to do this for us. It seems we have seen a rise of issues caused by trailing slashes and this is exactly what I was worried about (even though they are probably not all related)
007c785
to
a84f31d
Compare
Are there any update about the status of this PR? We can't update Nuxt in our project as this issue is blocking us. At the moment we are stuck on |
I don't know the proper way how to fix my PR to be merged, so I just still waiting :) @pi0 could you help me with PR? |
Codecov Report
@@ Coverage Diff @@
## dev #6745 +/- ##
=======================================
Coverage 70.28% 70.28%
=======================================
Files 88 88
Lines 3749 3749
Branches 1019 1019
=======================================
Hits 2635 2635
Misses 906 906
Partials 208 208
Continue to review full report at Codecov.
|
Hi @crutch12 Sure we should merge this fix just as @pimlie mentioned, it is not only source of inconsistencies for baseURL and trailing slash handling. (like regression introduced by #6775 with 2.12). Do you mind adding an e2e test to cover this change? Meanwhile working to make a url utility package to use it on all places instead of depending on sanetization(s) |
Ok, I'll try |
@pi0 I've added some e2e tests for checking spa mode router.base (in browser, based on spa fixtures) Basically with/without trailing slash tests. Could you check it out? |
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.
Looks awesome! Thanks for your time @crutch12 💚
Types of changes
Description
Resolves: #6744
Checklist: