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

Ability to serve base page without trailing slash #9236

Closed
4 tasks done
benmccann opened this issue Jul 20, 2022 · 6 comments · Fixed by #10723
Closed
4 tasks done

Ability to serve base page without trailing slash #9236

benmccann opened this issue Jul 20, 2022 · 6 comments · Fixed by #10723
Labels
enhancement New feature or request p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Milestone

Comments

@benmccann
Copy link
Collaborator

benmccann commented Jul 20, 2022

Description

SvelteKit has a trailingSlash option which indicates whether we expect URLs to have a trailing slash or not. By default all URLs do not have a trailing slash. However, Vite forces a trailing slash on the base URL.

Implementing this would close #8770 and #8772

Suggested solution

Vite could accept base with or without a trailing slash so that the user can decide which they would prefer. However, this would be a breaking change as anything reading base from the config would now have to handle the possibility both of it having or not having a trailing slash

Alternative

Add a new trailingSlash option similar to SvelteKit's. I think it would likely only affect the base URL though for Vite

#8772 is open which affects this functionality as well, but would not work as a solution for SvelteKit as it always forces a trailing slash and would conflict with SvelteKit's trailingSlash option

Additional context

SvelteKit historically hasn't used the publicDir and base options, but does its own static asset serving. I put together a branch to use Vite's implementation, but some of our tests are failing with it due to this issue, which is a blocker for us.

https://github.com/sveltejs/kit/pull/5601/files#r923967104

Validations

@benmccann benmccann added enhancement New feature or request p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels Jul 20, 2022
@Rich-Harris
Copy link
Contributor

I don't think Vite needs a new option, it should just accept /foo as a valid pathname when base === '/foo', instead of inviting you to visit /foo/foo

@benmccann
Copy link
Collaborator Author

benmccann commented Jul 21, 2022

Yeah, I mentioned that option as well. It's a simpler user API, but likely to be a breaking change and harder for developers to use. I don't know which is better and am fine with either solution

@benmccann benmccann changed the title Option to serve base page without trailing slash Ability to serve base page without trailing slash Jul 23, 2022
@patak-dev patak-dev added this to the 4.0 milestone Oct 21, 2022
@benmccann
Copy link
Collaborator Author

benmccann commented Oct 21, 2022

This was discussed at today's meeting. It was suggested to put together a PR for it. We also discussed the possibility of a helper method for handling appending a path to a base URL. We were trying to figure out how to help frameworks migrate and thought one possibility might be that the helper could be backported to 3.2 so that frameworks can start adopting the helper and when we disable enforcement of the trailing slash there will not be an effect on the frameworks that have already moved to using the helper.

@benmccann
Copy link
Collaborator Author

#10590 was merged in order to append the URL to the base path in a safe manner throughout the codebase regardless of whether it ends with a / or not. Assuming I didn't miss any places we were doing that then hopefully we're a good chunk of the way there.

The first thing would be to change the config validation to not enforce a trailing /:

// ensure leading slash

Possibly the base middleware would need some small tweaks

https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/middlewares/base.ts

And we should add a test to ensure that a base URL with no trailing slash works as intended

@BenediktAllendorf
Copy link
Contributor

You missed the place where it says // prepend base (dev base is guaranteed to have ending slash) - and it took me forever to find that! ;)

Nevertheless, I've opened a PR for this and I think the basic functionally is there already. Some behavior has to be decided on and maybe someone else can help me with the remaining broken tests.

@benmccann
Copy link
Collaborator Author

That's great! Thank you so much @BenediktAllendorf! I might not be able to look at this week, but will definitely try to help get it in before the first 4.0 alpha release. Thanks for finding the place I missed! (I searched for everywhere config.base was used, but missed that one since it was destructured)

patak-dev pushed a commit that referenced this issue Nov 12, 2022
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Closes #9236
Closes #8770
Closes #8772
tony19 pushed a commit to tony19/vite-docs-template that referenced this issue Nov 12, 2022
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Closes vitejs/vite#9236
Closes vitejs/vite#8770
Closes vitejs/vite#8772
docschina-bot pushed a commit to vitejs/docs-cn that referenced this issue Nov 12, 2022
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Closes vitejs/vite#9236
Closes vitejs/vite#8770
Closes vitejs/vite#8772
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants