-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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(server): ensure consistency for url to file mapping in importAnalysis and static middleware #6518
Conversation
Looks like a good refactoring, but the windows fails look legit. So something is missing. |
Didn't notice the failing windows test after the approval 😅 but yeah that should be taken of too |
0a45f21
to
364b93f
Compare
I managed to get my windows vm broken and won't restore it before getting a new workstation set up (finally more ram). If someone wants to debug this in the meantime, go ahead. Otherwise it'll be a few days before i can |
364b93f
to
22618e2
Compare
debug output of
|
note the extra leading slash in front of the logged allowed directories. this is added here: vite/packages/vite/src/node/server/index.ts Line 696 in a012644
i'm not entirely sure why because path.resolve should always return an absolute path. another oddity i noticed is that the drive letter c is lowercase in the first but uppercase in the second entry. Is windows fs still case insensitive and do we have to make an insensitive comparison in that case? I'm this close to not ever going to touch anything windows again >< |
…code to determine filepath from url
…efore drive letter from allow dirs on windows
22618e2
to
919cea0
Compare
@bluwy notified me that normalizePath is public api so this would be a breaking change. i'll try to refactor it, gotta come up with a better idea first tho |
packages/vite/src/node/utils.ts
Outdated
if (isCaseInsensitiveFS) { | ||
normalized = normalized.toLowerCase() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't do the normalization to lowercase in this PR. We had a few requests to do so before but decided that it was better to ask the user to properly name paths. Something may work in windows but fail in linux if we had this in. Touching normalizePath
is also quite a big change. Let's check the rest of the issues without it
…for subdirectory access
.github/workflows/ci.yml
Outdated
@@ -67,6 +68,7 @@ jobs: | |||
run: pnpm run test-build -- --runInBand | |||
|
|||
lint: | |||
timeout-minutes: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you send these two timeouts in a separate PR @dominikg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done #6843
To play safe here, let's merge this one in the 2.9 beta (that given the number of PRs we already queued we should start soon anyways) |
Description
fixes #6516
includes slight update to
fsPathFromId
to avoid stripping FS_PREFIX if it isn't setAdditional context
there may be more at play here and deeper analysis/ more testing of the code in static middleware is warranted.
The way url, id and file system path are handled currently isn't transparent with hardcoded special treatment even outside of utils.
Maybe switching path to pathe could be an option to avoid some of the inconsistencies?
cc @antfu @patak-dev
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).