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

Local image resolution from external dns address #6128

Closed
1 task
arash16 opened this issue Feb 4, 2023 · 5 comments · Fixed by #8698
Closed
1 task

Local image resolution from external dns address #6128

arash16 opened this issue Feb 4, 2023 · 5 comments · Fixed by #8698
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: assets Related to the Assets feature (scope)

Comments

@arash16
Copy link

arash16 commented Feb 4, 2023

What version of astro are you using?

1.6.13

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

yarn

What operating system are you using?

Mac

Describe the Bug

I am developing a website on my mac system, for testing purposes I use my phone to browse it in my home wifi. My modem is configured to resolve my mac-hostname to it's address, so inside my phone I open for example http://arash-mac:3000. But inside my mac, I have other dns servers configured, which means it cannot resolve it's own address. Inside @astrojs/image there's a line that tries to resolve local images through whatever hostname that they are requested through. In this case, it tries to resolve http://arash-mac:3000/_image/etc instead of http://localhost:3000/_image/etc.

At following line of code, when isRemoteImage returns false, url.origin is used as is, which in this complex scenario cannot be resolved into local machine. I did fix it by patching it into new URL(transform.src, 'http://localhost:' + url.port), however I believe the code design is problematic on a different level (trying to fetch local images through http queries).

https://github.com/withastro/astro/blob/main/packages/integrations/image/src/endpoint.ts#L33

Link to Minimal Reproducible Example

not applicable

Participation

  • I am willing to submit a pull request for this issue.
@matthewp matthewp added the - P2: nice to have Not breaking anything but nice to have (priority) label Feb 8, 2023
@Princesseuh Princesseuh added the feat: assets Related to the Assets feature (scope) label Apr 10, 2023
@Princesseuh Princesseuh added - P3: minor bug An edge case that only affects very specific usage (priority) and removed - P2: nice to have Not breaking anything but nice to have (priority) labels Jul 27, 2023
@xriter
Copy link

xriter commented Jul 27, 2023

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ocszci

To continue the conversation from #7819 ...

@Princesseuh
The code in that endpoint is really annoying, but it kinda has to be this way due to SSR.. I'm not sure yet how we could make it >more reliable. Probably by reading some parts from the Astro config, I'd assume..

This problem is very annoying, as it now prevents us from using Assets in production, and have to fall back to 'normally' loading (unoptimised) images until this has been resolved. (In my view this is a P4: Important Bug)

I don't really understand why the _image endpoint uses fetch to get the image over the network again in the first place. Especially because this IS SSR. Knowing the image path, why can't the image simply be loaded using the file system? In the end all it seems to need is a Buffer. (I guess it's platform specific, but in Node one could just simply use fs?)

@Princesseuh
Copy link
Member

Princesseuh commented Jul 27, 2023

I don't really understand why the _image endpoint uses fetch to get the image over the network again in the first place. Especially because this IS SSR. Knowing the image path, why can't the image simply be loaded using the file system? In the end all it seems to need is a Buffer. (I guess it's platform specific, but in Node one could just simply use fs?)

There's multiple reasons, some of which the endpoint doesn't really handle but should eventually:

  • Your assets might not be on the same server as your endpoint (think assets hosted on CDN, endpoint somewhere else)
  • Works no matter the platform, otherwise we'd need a node, deno, cloudflare workers, edge etc specific entrypoint. We currently don't have a filesystem abstraction in Astro itself, so using fetch worked nicely.
  • Every adapter bundles files differently and from the endpoint, it's a bit challenging to know the shape of the filesystem

This problem is very annoying, as it now prevents us from using Assets in production, and have to fall back to 'normally' loading (unoptimised) images until this has been resolved. (In my view this is a P4: Important Bug)

This is not an optimal solution since it's a bit of work, but you could make your own endpoint in the meantime. It just has to call the image service's hooks and read from the filesystem like you suggest.

@xriter
Copy link

xriter commented Jul 27, 2023

Thank you @Princesseuh for clarifying. I guess I'll have to go the custom endpoint route then. (https://docs.astro.build/en/reference/image-service-reference/#local-services).

@matthewp
Copy link
Contributor

We've removed @astrojs/image from Astro and are encouraging people to us astro:assets instead. We won't be fixing @astrojs/image issues going forward, so closing, thanks!

@xriter
Copy link

xriter commented Aug 10, 2023

@matthewp This issue also applies to astro:assets though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: assets Related to the Assets feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants