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(remix-dev/vite, remix-react, remix-server-runtime): support custom basename #8145

Merged
merged 45 commits into from
Feb 6, 2024

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 27, 2023

Closes: #8052
References: #2891, #8077 (comment), #5236

This PR further extends #8141 to integrate react-router's basename option so that route definition and navigation urls don't require to be manually prefixed with custom basename.
Maybe I should've discussed with Remix team beforehand, but I thought knowing the amount of change required for this feature can be an important factor, so here it is.
Please let me know if there is a concern about the introduction of this feature (and/or a concern about this PR's approach).
Thanks!

summary of the changes

  • Add RemixVitePluginOptions.basename
  • Add ServerBuild.basename so that server-runtime can use it for createStaticHandler(..., { basename }) and also for a few logic outside of react-router (e.g. "X-Remix-Redirect").
  • Add __remixContext.basename so that client can use it for createBrowserRouter(..., { basename }).
  • Replace matchRoutes(routes, url) with matchRoutes(routes, url, basename).

Also the changes from #8141 are:

  • Replace req.url with req.originalUrl since react-router's StaticHandler assumes request url includes basename.
  • Add publicPath prefix to all client-visible url for vite dev.

questions

  • I noticed v7_prependBasename is introduced in react-router#10336. As far as my manual test goes (e.g. relative link, useNavigate, etc...), it seems to be working without this flag, but I could be missing something.

  • For now, I'm only testing with Vite plugin. It might be possible to support non-Vite Remix, but I haven't attempted that yet. Please let me know if it's desired to add support for non-Vite use cases as well.

  • Interaction between basename and publicPath options might be tricky. I was testing basename != publicPath case, but during dev, Vite server forces SSR request to come under the path of base (= publicPath) option, so it's required to have basename.startsWith(publicPath). On the other hand, I believe there's no restriction for "build + start" as long as the server is configured to handle the routes properly. So, I added this exact validation in resolvePluginConfig, but probably we can explore other approaches as well (for example, if publicPath is undefined but basename is given, then remix automatically sets publicPath = basename assuming that's the most common usage).

testing strategy

I setup a demo project in hi-ogawa/remix-vite-custom-base#1 and wrote a list of features I'm testing manually.
For playwright test here, I only added very simple one as a reference. If the feature is agreed upon, then I plan to expand the test cases.

Copy link

changeset-bot bot commented Nov 27, 2023

🦋 Changeset detected

Latest commit: d9a61c2

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/express Minor
@remix-run/dev Minor
@remix-run/react Minor
@remix-run/server-runtime Minor
@remix-run/serve Minor
@remix-run/testing Minor
@remix-run/cloudflare Minor
@remix-run/deno Minor
@remix-run/node Minor
@remix-run/cloudflare-pages Minor
@remix-run/cloudflare-workers Minor
@remix-run/architect Minor
create-remix Minor
remix Minor
@remix-run/css-bundle Minor
@remix-run/eslint-config Minor

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

@hi-ogawa hi-ogawa force-pushed the feat-remix-react-router-basename branch from ca9ab82 to 8039329 Compare November 27, 2023 05:15
@hi-ogawa hi-ogawa changed the title feat(remix-react, remix-server-runtime): support react-router basename (wip) feat(remix-react, remix-server-runtime): integrate react-router basename (wip) Nov 27, 2023
@hi-ogawa hi-ogawa changed the title feat(remix-react, remix-server-runtime): integrate react-router basename (wip) feat(remix-react, remix-server-runtime): integrate react-router basename Nov 27, 2023
@hi-ogawa hi-ogawa force-pushed the feat-remix-react-router-basename branch from 2236535 to 5126562 Compare November 27, 2023 07:36
@hi-ogawa hi-ogawa changed the title feat(remix-react, remix-server-runtime): integrate react-router basename feat(remix-dev, remix-react, remix-server-runtime): support custom basename Nov 27, 2023
@hi-ogawa hi-ogawa changed the title feat(remix-dev, remix-react, remix-server-runtime): support custom basename feat(remix-dev/vite, remix-react, remix-server-runtime): support custom basename Nov 27, 2023
@hi-ogawa hi-ogawa marked this pull request as ready for review November 27, 2023 10:32
@hi-ogawa hi-ogawa force-pushed the feat-remix-react-router-basename branch from bbf2abc to 01e201f Compare November 27, 2023 11:41
@jrestall
Copy link
Contributor

This is really awesome Hiroshi, as you mentioned and from my experience, it's definitely not super easy to host remix sites under a subfolder as the base url has to be manually added to every Link and redirect along with custom route generation. This will make it much easier thanks!

@brophdawg11
Copy link
Contributor

Thanks for the new tests @hi-ogawa! I merged dev in and took care of the merge conflicts and did some more local testing and things are looking good.

@markdalgleish Let's sync up next week on the comments above about base and publicPath.

I think for npm run dev - you mostly want them to be the same thing? I was having trouble getting them to work as different root relative paths (/base/ versus /base/other/).

With a custom express server publicPath is still useful to put your assets on a CDN and I was able to get it working (build/start) with base: "/base/" and publicPath: "http://localhost:3001/other".

I'm unsure if I'm missing a capability or if we want to just recommend (or even enforce) that they're the same in dev, but let you do whatever you want in prod.

@brophdawg11
Copy link
Contributor

Chatted with @markdalgleish and @pcattori and we realized that Vite's base is actually just Remix's publicPath (and is effectively referred to as that in the docs), and basename should be a separate thing after all and not coupled to base. It's totally possible in prod to use a router basename of /app/ but then use a vite base of http://cdn.example.com/assets - so we can't couple them together.

I updated the plugin such that:

  • Vite base sets ServerBuild.publicPath
  • remix({ basename }) passes through to the React Router basename config

It's slightly confusing to see base and basename so close together in the config since they do two different things. We're stuck with base from the vite side. We could change the remix plugin config value - but I hesitate to stray from the basename wording that so many React Router users already know.

  1. We tossed around potentially using routerBasename to differentiate it a bit more
  2. Pedro also suggested just base since then there's a difference from the bundler base (assets) and the Remix base (routes) - but that could be ambiguous and tricky to document

@kiliman
Copy link
Collaborator

kiliman commented Feb 1, 2024

I think using basename in the Vite config is fine. This is not a common usage, and those who need it will know what it means. And the fact that it maps to RR's basename makes it quite clear.

@dahei07
Copy link

dahei07 commented Feb 4, 2024

cound basename option be configured differently between client and server-side? because sometimes the reverse proxy server has path-rewrite option.

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Feb 4, 2024

cound basename option be configured differently between client and server-side? because sometimes the reverse proxy server has path-rewrite option.

Remix/React-router needs to manage a whole user-visible URL in the same way both on client and server, so I don't think your reverse proxy scenario is supported at the remix level.

Though I'm not too familiar with reverse proxy or path-rewrite, I would guess removing such path-rewrite option and letting remix app handle full url pathname would be a simpler way to make it work.

If reverse proxy rewrite is unavoidable for some reason, then I think it's still possible to compensate for that at your (express) custom server level, something like:

const app = express();

// do whatever necessary so that remix handler will see `req.url` with `basename`
app.use((req, _res, next) => {
  req.url = "/mybase" + req.url; 
  next();
})

// normal remix request handler
app.all("*", createRequestHandler({ ... }))

@brophdawg11 brophdawg11 merged commit 44d1cc6 into remix-run:dev Feb 6, 2024
9 checks passed
@hi-ogawa hi-ogawa deleted the feat-remix-react-router-basename branch February 6, 2024 22:58
Copy link
Contributor

🤖 Hello there,

We just published version 2.7.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!

Copy link
Contributor

🤖 Hello there,

We just published version 2.7.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!

@AJK74
Copy link

AJK74 commented May 2, 2024

Thanks for this - in vite.config.ts we're using

  base:"/shopify-setup-app/",
  plugins: [
    remix({basename: "/shopify-setup-app/",...})
  ]

and works nicely.

Just need some docs now! 😊

@brophdawg11
Copy link
Contributor

In case you haven't found them - we do have docs for basename, and we chose to link off to the Vite docs for base rather than regurgitate their information (and let it get stale/outdated). Is there something in particular you think is missing?

@dimw
Copy link

dimw commented May 5, 2024

Need some domain knowledge here, @brophdawg11: it looks like when the base and basename are set to /app/ (this exact value!), the remix vite:dev is failing to start with the following issue:

[vite] Pre-transform error: Failed to load url /entry.server.tsx (resolved id: /entry.server.tsx) in virtual:remix/server-build. Does the file exist?
[vite] Error when evaluating SSR module virtual:remix/server-build: failed to import "/entry.server.tsx"
|- Error: Failed to load url /entry.server.tsx (resolved id: /entry.server.tsx) in virtual:remix/server-build. Does the file exist?
    at loadAndTransform (file:///<redacted>/regular-test/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:53884:21)

[vite] Internal server error: Failed to load url /entry.server.tsx (resolved id: /entry.server.tsx) in virtual:remix/server-build. Does the file exist?
      at loadAndTransform (file:///<redacted>/regular-test/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:53884:21)
[vite] Pre-transform error: Failed to load url /root.tsx (resolved id: /root.tsx) in virtual:remix/server-build. Does the file exist?
[vite] Pre-transform error: Failed to load url /routes/_index.tsx (resolved id: /routes/_index.tsx) in virtual:remix/server-build. Does the file exist?
[vite] Internal server error: Failed to load url /entry.server.tsx (resolved id: /entry.server.tsx) in virtual:remix/server-build. Does the file exist?
      at loadAndTransform (file:///<redacted>/regular-test/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:53884:21)
[vite] Internal server error: Failed to load url /entry.server.tsx (resolved id: /entry.server.tsx) in virtual:remix/server-build. Does the file exist?
      at loadAndTransform (file:///<redacted>/regular-test/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:53884:21) (x2)

Works fine with any other values for base / basename.

Can you help me to understand whether it's a Remix plugin issue or Vite so that I can raise an issue accordingly?

Issue details described in this discussion: #9375.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.