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.

Sign up for GitHub

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

Jump to bottom

can jump 404 when that page does not exist #5701

Merged
merged 19 commits into from
Jan 10, 2023
Merged

can jump 404 when that page does not exist #5701

merged 19 commits into from
Jan 10, 2023

Conversation

JerryWu1234
Copy link
Contributor

Changes

fix bug #5535

Testing

Docs

@changeset-bot
Copy link

changeset-bot bot commented Dec 30, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: bb874aa

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: integration Related to any renderer integration (scope) label Dec 30, 2022
bluwy
bluwy reviewed Jan 5, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I have one nit but this looks good to me! Could we add some tests too to make sure we don't make regressions in the future?

packages/integrations/node/src/middleware.ts Outdated Show resolved Hide resolved
@JerryWu1234
Copy link
Contributor Author

I have one nit but this looks good to me! Could we add some tests too to make sure we don't make regressions in the future?

Let me research how to add a test for this code

@JerryWu1234
Copy link
Contributor Author

@bluwy I added some tests

bluwy
bluwy reviewed Jan 9, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

The new test looks great! Thanks for adding it. I have a few comments below but once done it should be all set.

packages/integrations/node/test/api-route.test.js Outdated Show resolved Hide resolved
packages/integrations/node/package.json Outdated Show resolved Hide resolved
.changeset/red-snakes-fetch.md Outdated Show resolved Hide resolved
@JerryWu1234
Copy link
Contributor Author

The new test looks great! Thanks for adding it. I have a few comments below but once done, it should be all set.

sorry about that,

The new test looks great! Thanks for adding it. I have a few comments below but once done it should be all set.

My bad. I haven't rechecked.
I will fix it as soon as possible

@JerryWu1234 JerryWu1234 requested a review from a team as a code owner January 10, 2023 08:22
@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: lit Related to Lit (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: solid Related to Solid (scope) labels Jan 10, 2023
@github-actions github-actions bot added pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) feat: markdown Related to Markdown (scope) labels Jan 10, 2023
@github-actions github-actions bot removed pkg: create-astro Related to the `create-astro` package (scope) pkg: solid Related to Solid (scope) feat: markdown Related to Markdown (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) pkg: astro Related to the core `astro` package (scope) pkg: lit Related to Lit (scope) pkg: react Related to React (scope) pkg: preact Related to Preact (scope) labels Jan 10, 2023
@sarah11918
Copy link
Member

Docs was pinged on this one, but I don't see anything needed from us here! I'm removing the review request, but if I missed something I should be looking at, just let me know!

@sarah11918 sarah11918 removed the request for review from a team January 10, 2023 12:42
bluwy
bluwy approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for making the fix.

@bluwy bluwy merged commit 9869f2f into withastro:main Jan 10, 2023
@astrobot-houston astrobot-houston mentioned this pull request Jan 10, 2023
Merged
@JerryWu1234 JerryWu1234 deleted the fix-404 branch January 11, 2023 07:00
@bluwy bluwy mentioned this pull request Jan 13, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluwy bluwy bluwy approved these changes

Assignees
No one assigned
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JerryWu1234 @sarah11918 @bluwy