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(prerender): Encode URLs to preserve "," in X-Nitro-Prerender Header #701

Closed
wants to merge 2 commits into from

Conversation

posixpascal
Copy link
Contributor

@posixpascal posixpascal commented Jan 3, 2023

When using IPX provider in combination with placeholder attribute the final URL that will get added to the X-Nitro-Prerender header contains a plain comma.

Due to the way nitropack currently works this will break static generation of these images due to this LOC in nitropack:

_links.push(...header.split(",").map((i) => i.trim()));

You can reproduce the issue yourself with the latest NPM version of @nuxt/image-edge (^1.0.0-27840416.dc1ed65):

<template>
<div>
     <nuxt-img src="images/logo.svg" /> 
     <nuxt-img src="images/logo.svg" placeholder />
</div>
</template>

When using the above vue component only one image will get statically generated when running yarn run generate.
The placeholder image gets lost due to the URL being /_ipx/q_50,s_10x10/images/logo.svg which nitropack reads as:

  • /_ipx/_50
  • s_10x10/images/logo.svg

This bug is (closely) related to the following issues:

I've also opened a companion PR on nitropack here: nitrojs/nitro#799

@netlify
Copy link

netlify bot commented Jan 3, 2023

👷 Deploy request for nuxt-image-v1 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit b588cb7

@posixpascal posixpascal changed the title fix(prerender): Encode URLs to preserve "," fix(prerender): Encode URLs to preserve "," in X-Nitro-Prerender Header Jan 3, 2023
@pi0
Copy link
Member

pi0 commented Jan 3, 2023

Thanks for the PR @posixpascal ❤️ We shall merge this PR after upstream nitrojs/nitro#799 got merged alongside with Nuxt 3.1 release awaiting Nitropack v2.

@pi0 pi0 added the pending label Jan 3, 2023
@pi0
Copy link
Member

pi0 commented Jan 3, 2023

Hmm thinking more, in the meantime, we might have a faster workaround. IPX supports & in addition to , for separators (https://github.com/unjs/ipx/blob/ca4d06705ccded4ddfbb453aaf5cb404652e5b0f/src/middleware.ts#L8). Can you please try if we can use an inline replace to only encode , as & in path segments? (IPX itself also decodes, perhaps we can completely encode that segment even and keep using ,)

@posixpascal
Copy link
Contributor Author

It does indeed work by just replacing , with & and without doing the additional decode on nitropack.

I still think a decode on nitropack is necessary when the filename themselves contain , but for now replacing , with & is a good workaround.

I've updated the PR. Let me know if you know a better place to solve the problem as I'm not familiar with the code base yet.

@pi0
Copy link
Member

pi0 commented Jan 23, 2023

Hey. Sorry for delay on this. Since Nitro v2 and Nuxt 3.1 with fix are coming soon (today/tomorrow) we can use more proper upstream fix in nitrojs/nitro#799.

@pi0 pi0 closed this Jan 23, 2023
@pi0 pi0 reopened this Jan 23, 2023
@posixpascal
Copy link
Contributor Author

posixpascal commented Jan 23, 2023

I've added the required codestyle changes.

@Zielgestalt
Copy link

@posixpascal I can confirm that the images are now generated with '&' as separator in the folders. That's great! Thanks.
But in the html-files the src is still with a ',' - so the images are not found. Maybe this is another step and not part of this pull request? If so - sorry for disturbing ;-)

@pi0
Copy link
Member

pi0 commented Jan 27, 2023

Thanks for PR! Landed via #725

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants