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.

Sign up for GitHub

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

Jump to bottom

fix(remix-dev/vite): use /@fs/ path to support default server/client entry outside of app root #7913

Merged
merged 3 commits into from
Nov 7, 2023
Merged

fix(remix-dev/vite): use /@fs/ path to support default server/client entry outside of app root #7913

merged 3 commits into from
Nov 7, 2023

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 5, 2023 •
edited
Loading

Closes: #7722

While looking at devtools network tab, I just spotted vite itself seems to be using /@fs/ url to define globals (the url is something like http://localhost:5173/@fs/home/hiroshi/code/tmp/remix-pnpm-workspace/node_modules/.pnpm/vite@4.5.0/node_modules/vite/dist/client/env.mjs cf. vitejs/vite#12471).
So, I came to this idea of doing something similar to support default entries outside of remix app root.

  • Docs
  • Tests

Testing Strategy:

I created a simpler reproduction in https://github.com/hi-ogawa/demo-remix-vite-pnpm-workspace and verified this fix using pnpm patch hi-ogawa/demo-remix-vite-pnpm-workspace@6d18e91

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 5, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 60665ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

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

@MichaelDeBoey MichaelDeBoey changed the title fix(vite): use /@fs/ path to support default server/client entry outside of app root fix(remix-dev/vite): use /@fs/ path to support default server/client entry outside of app root Nov 6, 2023
@pcattori pcattori self-assigned this Nov 6, 2023
@pcattori
fix(vite): serve any file outside of project root via /@fs
60665ff 
By default, Vite prevents access to files outside the workspace root
(when using workspaces) or outside of the project root (when not using
workspaces) unless user explicitly opts into it via Vite's `server.fs.allow`.
@pcattori
Copy link
Contributor

pcattori commented Nov 7, 2023

@hi-ogawa thanks for your contribution! I extended your solution to work for any files outside of the Remix project root, not just the entry client/server files since server.allow.fs already prevents serving files outside of the workspace by default.

@pcattori pcattori merged commit 2c36e11 into remix-run:dev Nov 7, 2023
9 checks passed
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Nov 7, 2023

@pcattori Sounds good! Thanks for adjusting the code.

@hi-ogawa hi-ogawa deleted the fix-vite-pnpm-default-entry branch November 7, 2023 01:20
@hi-ogawa hi-ogawa mentioned this pull request Nov 9, 2023
Closed
1 task
@jrestall jrestall mentioned this pull request Nov 10, 2023
Closed
1 task
@markdalgleish markdalgleish mentioned this pull request Nov 13, 2023
Closed
1 task
@github-actions github-actions bot mentioned this pull request Nov 14, 2023
Merged
@github-actions GitHub Actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.3.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot mentioned this pull request Nov 16, 2023
Merged
@github-actions GitHub Actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.3.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@hi-ogawa hi-ogawa mentioned this pull request Nov 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdalgleish markdalgleish Awaiting requested review from markdalgleish

@pcattori pcattori Awaiting requested review from pcattori

Assignees

@pcattori pcattori

Labels
CLA Signed package:dev vite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hi-ogawa @pcattori @MichaelDeBoey