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

fix: _image endpoint returning a 404 for local images #8828

Merged
merged 5 commits into from Oct 17, 2023
Merged

fix: _image endpoint returning a 404 for local images #8828

merged 5 commits into from Oct 17, 2023

Conversation

rishi-raj-jain
Copy link
Contributor

fix #8735

@changeset-bot
Copy link

changeset-bot bot commented Oct 13, 2023 •
edited

🦋 Changeset detected

Latest commit: 22b9526

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 Oct 13, 2023
ematipico
ematipico reviewed Oct 16, 2023
View reviewed changes
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.

Is it possible to add a test?

@rishi-raj-jain
Copy link
Contributor Author

rishi-raj-jain commented Oct 16, 2023 •
edited

@ematipico

I'll take a look at this later today, but I guess there must be a test for this already, and I just need to update it?

@Princesseuh
Copy link
Member

Princesseuh commented Oct 17, 2023 •
edited

Adding a test is a bit complicated because this specific issue happens for multiple reasons:

  • Windows paths weren't handled properly
  • which..
  • made it use the fallback solution (using a fetch)
  • which...
  • failed because of a unusual setup (using https in dev with another Vite plugin)

@ematipico
Copy link
Member

What's your suggestion here @Princesseuh ? Should manual testing be enough?

@Princesseuh
Copy link
Member

What's your suggestion here @Princesseuh ? Should manual testing be enough?

I'd say it's okay for now, it's a quite precise setup. We have a lot of tests using the endpoint, more general failures will be caught.

@rishi-raj-jain
Copy link
Contributor Author

Awesome! Shall I update the branch or we're gonna proceed with merging?

@Princesseuh
Copy link
Member

Awesome! Shall I update the branch or we're gonna proceed with merging?

I'll merge it, thank you for your contribution!

@Princesseuh Princesseuh merged commit 11f45b9 into withastro:main Oct 17, 2023
13 checks passed
@rishi-raj-jain rishi-raj-jain deleted the patch-4 branch October 17, 2023 10:13
@rishi-raj-jain
Copy link
Contributor Author

My pleasure, on my way to fixing more of P3 bugs.

@astrobot-houston astrobot-houston mentioned this pull request Oct 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ematipico ematipico ematipico approved these changes

@Princesseuh Princesseuh Princesseuh approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

_image endpoint returning a 404 for local images
3 participants
@rishi-raj-jain @Princesseuh @ematipico