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

Vite strips base URL from request #9234

Closed
7 tasks done
benmccann opened this issue Jul 20, 2022 · 6 comments
Closed
7 tasks done

Vite strips base URL from request #9234

benmccann opened this issue Jul 20, 2022 · 6 comments
Labels
breaking change p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@benmccann
Copy link
Collaborator

benmccann commented Jul 20, 2022

Describe the bug

Vite strips the base URL to avoid other middlewares from having to worry about the base URL:

req.url = url.replace(devBase, '/')

Unfortunately, it doesn't really save us from having to worry about the base path and creates a number of broken / unexpected behaviors. E.g. our 404 messages print Not found: /whatever instead of Not found: /basepath/whatever.

While SvelteKit could change its own middleware, that's impossible off-the-shelf middlewares that users might. Some examples that are broken with Vite:

SvelteKit does not use Vite's static asset serving because of this, but I'd love just use Vite's instead of recreating our own

Reproduction

req.url = url.replace(devBase, '/')

System Info

`main`

Used Package Manager

npm

Logs

No response

Validations

@benmccann benmccann added bug pending triage breaking change p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed pending triage labels Jul 20, 2022
@benmccann benmccann added p2-edge-case Bug, but has workaround or limited in scope (priority) and removed p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels Oct 25, 2022
@benmccann
Copy link
Collaborator Author

We came up with an easy workaround for this: sveltejs/kit#7343

That's enough for SvelteKit. It could still cause problems for users if they have generic middlewares they want to use, but this is lower in priority now

@bluwy
Copy link
Member

bluwy commented Oct 30, 2022

We discussed this briefly last meeting (I think Ben was there too) that reverting the req.url change would be a big breaking change to all downstream SSR frameworks, and something we're trying to avoid in Vite 4.

I didn't know of the req.originalUrl property but it feels like that is the solution if someone wants to access the original url. The express docs also mentions mutating req.url as a common pattern. So maybe we don't need to change this in Vite?

@benmccann
Copy link
Collaborator Author

I wouldn't object to closing this. In any case, it's low enough priority now that I'm not going to pursue it

@patak-dev
Copy link
Member

About req.originalUrl, your original PR could have helped when using generic Vite middlewares, because we could tell people to use req.url (no base) or req.originalUrl as needed. The one you end up merging will make middlewares that expect no base fail. An option if you still want to continue this path is to still add a full URL and baseless URL helpers so middlewares can be more explicit. I'm a bit worried that SvelteKit and other projects will not be able to share plugins that use .configureServer and expect req.url to be baseless

@benmccann
Copy link
Collaborator Author

The one you end up merging will make middlewares that expect no base fail.

I had thought about that, but didn't think it'd be an issue as it'd only cause trouble if they come after the SvelteKit middleware and I believe none do, so it should be safe. We could restore the URL Vite provides at the end of processing if this turns out not to be the case.

@patak-dev
Copy link
Member

patak-dev commented Oct 31, 2022

If the SvelteKit middlewares are always expected to be last, then I think you could close this issue @benmccann, the PR you merged then looks good for now to me too 👍🏼

@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
Archived in project
Development

No branches or pull requests

3 participants