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.

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(rerouting): check that the new route is different #8648

Merged
merged 3 commits into from Sep 28, 2023

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Sep 23, 2023

Changes

Testing

Added 4 cases based on what I saw people encounter in the issue and on discord.

Docs

Does not affect usage.

@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2023

🦋 Changeset detected

Latest commit: f522ad2

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 23, 2023
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I wonder if we can add a test case, the same we have in the issue

@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 24, 2023

Let's put a potential endless loop in our CI 🥳

@@ -220,6 +220,7 @@ export async function handleRoute({
let response = await pipeline.renderRoute(renderContext, mod);
if (response.status === 404 && has404Route(manifestData)) {
const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline);
// if (fourOhFourRoute?.route !== options.route)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commenting out the fix to see what happens to CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI times out. Logs for astro:test never appear. It wouldn't be possible to tell if the cases failed.

@ematipico
Copy link
Member

Let's put a potential endless loop in our CI 🥳

Shouldn't Astro catch these loops? 🤔

@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 25, 2023

They seem to be synchronous, so there's not stopping them.

@ematipico
Copy link
Member

Well, let's not overthink it then. You tested manually :)

@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 25, 2023

And we don't have the bug in our codebase anymore! 🥳

at least this particular instance of it

@lilnasy lilnasy merged commit cfd895d into withastro:main Sep 28, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 28, 2023
@lilnasy lilnasy deleted the fix-8257 branch September 30, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants