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: handle encoded base paths #17577

Merged
merged 7 commits into from
Aug 1, 2024
Merged

fix: handle encoded base paths #17577

merged 7 commits into from
Aug 1, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jun 27, 2024

Description

supersedes and closes #16412

Due to the changes from #15311 (and tweaks thereafter) (Vite 5.2), I believe it broke most cases of base that contains encoded base paths, e.g. /foo%20bar/, which Vite encodes internally even if you pass /foo bar/.

I'm somewhat relieved that we didn't get a lot of reports of this, because base paths that look like this are hard 🥲


Because resolvedConfig.base is a URL-encoded string, when we merge it with some un-encoded strings, and encode the merged string, the base portion will get double encoded. The solution in this PR is to decode the base first.

In the future, maybe we can revisit if we want to have resolvedConfig.base be decoded only, or introduce a new resolvedConfig.decodedBase? Or we can say we don't support strange base paths.

NOTE: I also updated the code in plugin-legacy together as I noticed it while searching the codebase.

@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release labels Jun 27, 2024
Copy link

stackblitz bot commented Jun 27, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is largely a copy of the other test files.

@patak-dev
Copy link
Member

/ecosystem-ci run

@patak-dev
Copy link
Member

Should we have a internal config._decodedBase to avoid all the new decodeURI calls?

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 5097e9f: Open

suite result latest scheduled
analogjs failure success
astro failure failure
nuxt failure failure
sveltekit failure failure
vike failure failure
vite-plugin-react-pages failure failure
vitest failure failure

histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

@patak-dev
Copy link
Member

/ecosystem-ci run analogjs

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 5097e9f: Open

suite result latest scheduled
analogjs failure success

@bluwy
Copy link
Member Author

bluwy commented Jul 23, 2024

Should we have a internal config._decodedBase to avoid all the new decodeURI calls?

I was wondering that in the PR description too. I think I can refactor to that if we think it's cleaner.

Also, looks like I need to investigate the analog fail (or seems like they recently merged something)

@bluwy bluwy added this to the 5.4 milestone Jul 24, 2024
@hi-ogawa
Copy link
Collaborator

When running the playground, I'm seeing two Pre-transform error. Are these expected?

$ pnpm -C playground/assets dev:encoded-base
...
  VITE v5.3.5  ready in 94 ms

    Local:   http://localhost:9524/foo%20bar/
    Network: use --host to expose
    press h + enter to show help
6:46:19 PM [vite] Pre-transform error: No matching HTML proxy module found from /foo bar/index.html?html-proxy&index=4.js
6:46:19 PM [vite] Pre-transform error: Failed to load url /foo bar/asset/main.js (resolved id: /foo bar/asset/main.js). Does the file exist?

@bluwy
Copy link
Member Author

bluwy commented Jul 31, 2024

I just pushed a fix to handle them. Looks like we were passing urls with decoded base, and then trying to strip with an encoded base, causing server.warmupRequest to not understand the path.

I'm not particularly happy with my fix as encoded and decoded urls/base are slightly more intertwined, but I think it's probably sufficient for now.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 7564c68: Open

suite result latest scheduled
astro failure failure
nuxt failure failure
qwik failure failure
vike failure failure
vite-plugin-react-pages failure failure
vitest failure failure

analogjs, histoire, ladle, laravel, marko, previewjs, quasar, rakkas, remix, sveltekit, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

@bluwy bluwy merged commit 720447e into main Aug 1, 2024
11 checks passed
@bluwy bluwy deleted the strange-base branch August 1, 2024 02:53
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) regression The issue only appears after a new release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants