kiliman / remix-express-vite-plugin

This package includes a Vite plugin to use in your Remix app. It configures an Express server for both development and production using TypeScript.
118 stars 6 forks source link

Middleware won't run if route doesn't have a loader function #17

Closed VHall1 closed 3 months ago

VHall1 commented 3 months ago

Hello! While implementing the requireAuth middleware provided in the example app, I found an issue I'm not 100% sure is a bug or a known issue / Remix limitation. I couldn't find anything mentioning this issue while reading the docs, so thought I should ask.

tldr: When navigating to a route that uses middleware, if the route doesn't have a loader function defined, the middleware won't be called unless a full document reload is done.

For example, using the following implementation, the middleware won't be called:

// _index.tsx
export default function Index() {
  return <Link to="/protected">protected route</Link>;
}

// protected.tsx
export const middleware = [requireAuth];

export default function Protected() {
  return <p>hello</p>;
}

This workaround however seems to resolve the issue:

// _index.tsx
export default function Index() {
  return <Link to="/protected">protected route</Link>;
}

// protected.tsx
export const middleware = [requireAuth];

export function loader() {
  return null
}

export default function Protected() {
  return <p>hello</p>;
}

Passing the reloadDocument prop to link also seems to resolve the issue.

VHall1 commented 3 months ago

Poking around the express server for a bit, and seems like app.all("*") doesn't get requests if the route doesn't have a loader, so wondering if Remix's link component just does client-side navigation if no server logic is required

VHall1 commented 3 months ago

Sorry for the ramble, haven't actually looked this closely at how Remix handles client/server bundles before, so finding things as I go.

From what I've seen so far, I'm starting to believe this might be a limitation rather than a bug in this package, as remix won't handle requests on the server if the route doesn't have a loader (among other criteria), which I guess is something we'll just have to wait until they release their official implementation to iron this out?

kiliman commented 3 months ago

I did some research, and you are correct. Unfortunately, this is how Remix works.

The first time you make a document request is to /. Remix will call loaders [root, _index].

When you make a client navigation to /protected, Remix will look for loaders for the new route. This would still be [root, _index] (no protected loader). Since the list of loaders for the new route already matches what is currently in context, Remix doesn't need to make a server fetch and simply renders the new route.

Once you add a loader, the loader set is [root, _index, protected]. This time, the loaders are different, and Remix will make a fetch.

Since my middleware implementation doesn't modify the Remix code and relies on the server fetch, it will only execute if the leaf routes have a loader.

To fully support middleware, I would need to patch the core code.

VHall1 commented 3 months ago

Yep, that's a bummer. I think it might be a good idea to mention this somewhere in the docs since it might not be immediately obvious to people why their middleware isn't working as expected if they don't define a loader. Happy to type something up and submit a PR if you think that would be helpful.

VHall1 commented 3 months ago

Moved to remix https://github.com/remix-run/remix/issues/9587