-
Notifications
You must be signed in to change notification settings - Fork 2.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(vite): apply normalizePath
to default entry file outside of project directory
#8057
fix(vite): apply normalizePath
to default entry file outside of project directory
#8057
Conversation
🦋 Changeset detectedLatest commit: 02f7588 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
normalizePath
for out-of-tree client entry file urlnormalizePath
to out-of-tree client entry file url
packages/remix-dev/vite/plugin.ts
Outdated
@@ -91,7 +91,7 @@ const resolveFileUrl = ( | |||
// Vite will prevent serving files outside of the workspace | |||
// unless user explictly opts in with `server.fs.allow` | |||
// https://vitejs.dev/config/server-options.html#server-fs-allow | |||
if (!isWithinRoot) return `/@fs` + filePath; | |||
if (!isWithinRoot) return path.posix.join(`/@fs`, normalizePath(filePath)); |
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.
The idea is to just mimic what vite does here
path.posix.join(FS_PREFIX, normalizePath(ENV_ENTRY))
I'm not really familiar with Window quirks, so it's not likely I can figure this out myself. |
This patch does, in fact, solve the issue described here: #7722 (comment) |
@alindsay55661 Thanks for testing! I had a feeling that there might be still something wrong, but if it works, then remix team might consider merging this PR without verifying on CI. I'll re-open this and wait for remix team's response. |
normalizePath
to out-of-tree client entry file urlnormalizePath
to default entry file outside of project directory
5fd9b74
to
63aa9f7
Compare
`/@fs` prefix works fine even if path has a Windows volume like `C:\\`
63aa9f7
to
02f7588
Compare
@pcattori @brophdawg11 @hi-ogawa This did not merge correctly. The change is absent from @remix-run/[email protected]: remix/packages/remix-dev/vite/plugin.ts Line 94 in b63c044
As I am following this closely, I installed the pre-release and noticed the same error occured. |
@alindsay55661 our GitHub bot gets a bit confused when changes land in |
🤖 Hello there, We just published version Thanks! |
Confirmed, looks great, really looking forward to the 2.4.0 release. |
🤖 Hello there, We just published version Thanks! |
Closes: #7722 (comment) (follow up of #7913)
I think this issue is because of the lack of
normalizePath
.In https://github.com/hi-ogawa/remix/pull/1/files, I'm trying to verify this works on Windows, but it seems there is still something wrong.
I see
SyntaxError: Unexpected token '<'
in https://github.com/hi-ogawa/remix/actions/runs/6919035654/job/18821929935#step:7:328 which indicates transform not being run for client entry.