-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: include base path in path to start script #8651
Conversation
🦋 Changeset detectedLatest commit: f5d0ac6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Is there a test we can add for this? |
This one is tricky to add a test for because it will pretty much work as it is today. The only reason it's broken in the linked issue is because the users are accessing the site through a proxy, but I don't want to setup a proxy just for the test. We could write a test that looks for the string |
Nah, that's probably not a good test. The reason I think we ought to have a test is that it honestly feels like a Vite bug that |
(I feel like |
I agree with that. I think the test would be best as part of a PR to change the behavior in Vite. It'd be hard to change the behavior at the moment as users might be relying on it via their Vite plugins and it'd be a breaking change. This is highly related to vitejs/vite#7363 which I just linked to in the code comment in #8683. It's probably my top request for Vite 5 so with any luck I'll get to fixing it then now that I cleaned up all the other base path stuff in Vite 4 |
I added a comment so that no one unknowingly breaks things by changing the code because they're unaware the base is needed |
fixes #2958