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

feat: base without trailing slash #10723

Merged
merged 22 commits into from Nov 12, 2022

Conversation

BenediktAllendorf
Copy link
Contributor

@BenediktAllendorf BenediktAllendorf commented Oct 30, 2022

Description

Allows setting a base without trailing slash. Closes #9236. Closes #8770. Closes #8772

I assumed, that base = "/foo/" and base = "/foo" should work equally for all sub-urls and only differ in the behavior when calling only the base (e.g., "localhost/foo" vs "localhost/foo/"). This means, that even without the trailing slash, all generated paths look like localhost/foo/css/... and not like localhost/foocss/....

Additional context

I used playground/assets for testing and reused that code (already available with /foo/ as base). There is already some duplicated code between relative-base and runtime-base, which probably could be combined altogether (?).

Not all tests are successful yet:

  • pnpm run test-build assets leads to AssertionError: expected '/non-existent' to include '/foo/non-existent'. The problem is, that the path is not available and therefore, will be resolved at runtime - but this basically means, URL("non-existent", "/foo") will be called (instead of URL("non-existent", "/foo/") as it would do with a trailing-slash-base. I'm not sure, what the expected/desired behavior here would be?
  • pnpm run test-serve assets
    • AssertionError: expected 'url("http://localhost:5173/foo/@fs/ho…' to include '/foo/nested/asset.png' and AssertionError: expected 'http://localhost:5173/foo/@fs/home/be…' to include '/foo/nested/asset.png' are both related to "@fs"-imports
    • AssertionError: expected 'url("http://localhost:5173/nested/ass…' to include '/foo/nested/asset.png' reveals a problem with inline-CSS.

For test-serve, I'm not sure where those problems occur (and why I don't see that neither with pnpm run dev nor pnpm run build/preview).


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@benmccann benmccann added this to the 4.0 milestone Oct 31, 2022
@benmccann benmccann added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Oct 31, 2022
BenediktAllendorf and others added 2 commits November 3, 2022 17:34
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@bluwy
Copy link
Member

bluwy commented Nov 4, 2022

We have discussed this in the last team meeting and agree that /base should allow accessing from localhost:4173/base and /base/ would be localhost:4173/base/ like the current behaviour.

An implementation detail we want to make sure is that the ResolvedConfig should still append a / if /base (but no warning), to preserve compatibility with plugins that uses the base in ResolvedConfig. To differentiate between if the user specified a trailing / or not, we can have a separate internal property in ResolvedConfig to track it, as this information is only needed by the server.

@benmccann
Copy link
Collaborator

@BenediktAllendorf I finally figured out that most of the failing tests were not because of any code changes, but the test themselves. Once you put the test in a sub-directory they no longer pass. I changed the test setup to put the new tests alongside the old tests

@benmccann
Copy link
Collaborator

Hmm, the way I setup the tests seemed to cause them to collide with each other. I just removed them and instead edited the base path on the existing tests to not have a trailing slash. Since there are so many other tests that have a base path with trailing slash we should still have good coverage

benmccann
benmccann previously approved these changes Nov 12, 2022
@@ -323,6 +323,7 @@ export type ResolvedConfig = Readonly<
inlineConfig: InlineConfig
root: string
base: string
rawBase: string
Copy link
Member

Choose a reason for hiding this comment

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

Let's mark rawBase as internal. I would prefer to have this option as you did here, but there was a push in the last team meeting to go there only when there is a real use case out of the Vite core middleware.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Nov 12, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ❌ failure
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ❌ failure
rakkas ✅ success
svelte ❌ failure
vite-plugin-ssr ❌ failure
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@patak-dev
Copy link
Member

vite-ecosystem-ci looks the same as before this PR, except for Svelte @benmccann but I expect that you are aware or ok with this to move forward.

@benmccann
Copy link
Collaborator

Thanks for the review @patak-dev ! I marked the new field as internal. The svelte failure is unrelated and looks to be a network flake during CI setup, so I'm not worried about that one

patak-dev
patak-dev previously approved these changes Nov 12, 2022
@patak-dev patak-dev enabled auto-merge (squash) November 12, 2022 20:06
@patak-dev patak-dev merged commit 8f87282 into vitejs:main Nov 12, 2022
@rssfrncs
Copy link

rssfrncs commented Jan 9, 2023

Hi is this released?

@patak-dev
Copy link
Member

@rssfrncs yes, in Vite v4+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Ability to serve base page without trailing slash serve: with base: '/docs/', should /docs work as /docs/?
5 participants