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): decode generated routes #1914

Merged
merged 3 commits into from
Nov 16, 2023
Merged

fix(prerender): decode generated routes #1914

merged 3 commits into from
Nov 16, 2023

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Nov 15, 2023

πŸ”— Linked issue

resolves nuxt/nuxt#22660
closes nuxt/nuxt#24320

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

When a link is rendered in HTML (e.g. by vue router) with URI components, Nitro currently does not decode these when requesting the URL, which can cause double encoding and a 404 from vue-router.

Instead we should likely decode the link at source, just as we do with headers.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added bug Something isn't working prerender labels Nov 15, 2023
@danielroe danielroe requested a review from pi0 November 15, 2023 22:26
@danielroe danielroe self-assigned this Nov 15, 2023
@pi0
Copy link
Member

pi0 commented Nov 16, 2023

Can you please also add a test? πŸ™πŸΌ (i am also not sure about this because we might wrongly decode/change original links. we could make it opt-in for nuxt as vue-router supports encoding for example but test case would be useful)

@danielroe
Copy link
Member Author

danielroe commented Nov 16, 2023

The source of the double encoding is from this line:

nitro/src/prerender.ts

Lines 168 to 169 in 466535a

// Fetch the route
const encodedRoute = encodeURI(route);

I think if we are forcibly going to encode the URI (see #891) it makes sense to decode scanned links first.

@danielroe
Copy link
Member Author

Okay, pushed a change that means route is always decoded. This covers both scanned links and link headers, as well as ensuring that we write the decoded version to the file system. βœ…

@pi0 pi0 changed the title fix(prerender): decode links found in scanned html fix(prerender): decode generated urls Nov 16, 2023
@pi0 pi0 changed the title fix(prerender): decode generated urls fix(prerender): decode generated routes Nov 16, 2023
@pi0 pi0 merged commit a3120b6 into main Nov 16, 2023
5 checks passed
@pi0 pi0 deleted the fix/decode-paths branch November 16, 2023 20:21
@pi0 pi0 mentioned this pull request Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working prerender
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong route paths generated with files with special characters
2 participants