remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
30.14k stars 2.55k forks source link

`v3_lazyRouteDiscovery` does not respect `Vite#base` configuration #10124

Open hanford opened 1 month ago

hanford commented 1 month ago

Reproduction

I’ve noticed that in the latest Remix release, v2.13.1, v3_lazyRouteDiscovery does not respect Vite’s base configuration.

After looking into the source code, it appears that while lazyRouteDiscovery respects Remix’s basename, however the two configuration options are for different purposes.

Ideally, I’d like the __manifest file to be prefixed with Vite’s base configuration (aligning it with all other JS being served).

It's my understanding that all assets are served under this base (similar to how webpack publicPath) if provided; however, __manifest is not, which is causing an error.


An asset that works (respecting Vite's base) image

v3_lazyRouteDiscovery not respecting the base, thus 404'ing image

Reproduction: https://github.com/hanford/remix-fogOfWar-base

System Info

System:
    OS: macOS 15.0
    CPU: (10) arm64 Apple M1 Pro
    Memory: 110.59 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - /opt/homebrew/bin/node
    Yarn: 1.22.22 - /opt/homebrew/bin/yarn
    npm: 10.2.3 - /opt/homebrew/bin/npm
    pnpm: 7.32.4 - ~/.npm-global/bin/pnpm
    bun: 1.1.22 - ~/.bun/bin/bun
    Watchman: 2024.08.12.00 - /opt/homebrew/bin/watchman
  Browsers:
    Chrome: 129.0.6668.101
    Safari: 18.0
  npmPackages:
    @remix-run/dev: ^2.13.1 => 2.13.1
    @remix-run/node: ^2.13.1 => 2.13.1
    @remix-run/react: ^2.13.1 => 2.13.1
    @remix-run/serve: ^2.13.1 => 2.13.1
    vite: ^5.1.0 => 5.4.9

Used Package Manager

pnpm

Expected Behavior

__manifest should be servered from base + __manifest

Actual Behavior

__manifest doesn't respect Vite's base, which leads to a 404 breaking the future flag

brophdawg11 commented 1 month ago

I'm seeing a 404 on https://github.com/hanford/remix-fogOfWar-base - can you check the permissions on that?

After looking into the source code, it appears that while lazyRouteDiscovery respects Remix’s basename, however the two configuration options are for different purposes.

Can you elaborate on your use case? Knowing what you're expected dev/prod setups are would help us ensure we've got the use cases handled properly.

To my recollection, when we did our initial basename implementation we found that during vite dev, basename had to start with base for vite dev to be able to work at all and I thought we had a warning/error in there when it didn't. It looks like when basename === base and when basename.startsWith(base), these __manifest requests work as expected.

hanford commented 1 month ago

Made the repo public!

My situation is as follows, I have ~10 separate frontend applications being served on different paths...

example.com/employee/* <-- Application one
example.com/admin/* <-- Application two
example.com/feature/*  <-- Application three
...

image

We have a reverse proxy in front of our applications that looks at the path of an incoming requests, and proxies it to the correct place.

These applications link to one another, share session information, UI components, and from a users perspective, it's just one application.

We aren't using Remix's basename, because we don't want our Links to automatically be prefixed within the application. Instead we're naming all of our routes with what would be the basename. e.g.

admin-application/
  /app/routes/
    /admin-route-1.tsx
    /admin-route-2.tsx
    /admin-route-3.tsx

However, we are using Vite#base, so that our application artifacts (JS/other assets) are prefixed with the applications base path so at the router level we know where to send requests for it's assets.


So, we want to enable lazyRouteDiscovery, but need the __manifest.json request to be prefixed with basePath similar to other application artifacts otherwise at the router level, we don't which application the request should go to.


Hopefully this makes sense, happy to provide more details or try alternative Remix/Vite configurations. Today we're actually a full Next.js shop (with our own patches on top of Next.js) and using a combination of Next.js features that enable this whole setup to work. We're trying to emulate the current setup w/ Remix to hopefully migrate some (or all) applications to Remix.

jrestall commented 1 month ago

We also hit this issue when trying to adopt v3_lazyRouteDiscovery. A lot of our apps were created before basename support was added so are using expressjs and the remix routes config prop to add a custom basename to routes instead.

Looks like this means we'll need to adopt basename before migrating to v7. Quite a big change as the basename is passed manually everywhere to redirects and fetchers etc.

dj-rabel commented 1 day ago

Same for us over here. Basename is not an option for us as we have mutliple possible basenames, which are handled via an optional param in the route. But as we are behind a proxy, having all the remix/react/vite resource path on root is not possible as well. Vite base allows us to set a basename for all the dev and asset urls, while keeping the optional first route segment fully functional for other routes with differend basenames. So if this is not considered a bug which is getting fixed, we are out of options =/