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

Fixes an issue where view transitions to the 404-page did not work #9642

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

martrapp
Copy link
Member

@martrapp martrapp commented Jan 8, 2024

Changes

Changed the cloning of a Response in route.ts to retain the original headers.
With the headers the content type of the 404 page was lost.
The view transition loader expected text/html and did a full reload due to the missing header.
Closes #9629

Testing

new e2e test to confirm that we can transition to 404 / undefined pages without reloads

Docs

n.a. / bug fix

Copy link

changeset-bot bot commented Jan 8, 2024

🦋 Changeset detected

Latest commit: 931b7bd

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 pkg: astro Related to the core `astro` package (scope) docs pr A PR that includes documentation for review labels Jan 8, 2024
Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks for adding the simple to understand test as well.

@github-actions github-actions bot removed the docs pr A PR that includes documentation for review label Jan 8, 2024
@martrapp martrapp merged commit cdb7bfa into main Jan 8, 2024
13 checks passed
@martrapp martrapp deleted the mt/lost_headers branch January 8, 2024 15:23
@astrobot-houston astrobot-houston mentioned this pull request Jan 8, 2024
Comment on lines -373 to +376
response = new Response(response.body, { ...response, status });
response = new Response(response.body, {
status: status,
headers: response.headers
});
Copy link
Member

Choose a reason for hiding this comment

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

Can confirm that ...response seems to do nothing here 👍 We could also copy over statusText, but not a big deal I think

Copy link
Contributor

Choose a reason for hiding this comment

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

That was it originally. I advised against it.

#9642 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, looks like you're already one step ahead 😄

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.

View Transition to 404 loses state
4 participants