-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: Ensures that rewrite url contains base path #13233
FIX: Ensures that rewrite url contains base path #13233
Conversation
and fixes edge cases where rewrites were incorrectly 404ing
🦋 Changeset detectedLatest commit: b55b5f8 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
and fixes edge cases where rewrites were incorrectly 404ing
Not sure if this needs to be split up into 2 PRs, or if this was the best way to test the solution. |
CodSpeed Performance ReportMerging #13233 will not alter performanceComparing Summary
|
Thank you @joshmkennedy , I've seen that it's not the first contribution you provide to fix some bugs. If you're not aware of it, you can close bugs automatically by using specific messages from your PR description: https://github.com/gitbucket/gitbucket/wiki/How-to-Close-Reference-issues-and-pull-request Can you update you PR to reflect that? So we won't forget to close the issue next time you provide a fix :) |
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.
From your PR, it seems that we are fixing two bugs. If so, we can provide two changesets, where we thoroughly explain what was the bug. Plus, the changeset you provided isn't very descriptive at the moment. For example url
should be Astro.url
and ctx.url
.
Let's provide a good changelog for our users :)
What should the expected behaviour be when rewriting? Should The current behaviour is: I think it should probably 404, but that might be a breaking change. Here is an example of what I'm talking about. |
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
This PR fixes #13033
Changes
After the rewrite, the URL will contain the base path and then the new pathname.
The other issue that was brought up was inconsistencies with trailingSlash and when the base was included in the Astro.rewrite call.
Testing
I have added a new fixture and have added tests to the
/tests/rewrite.test.js
file that test and ensure the Astro.url pathname always contains the base path. This is tested both with and without the trailingSlash, also with the base being included and excluded in argument of Astro.rewrite.Docs