Middleware should run even if there is no loader

[infiniteluke]

The soon to ship middleware feature is currently coupled to the need to run a loader/action. So, routes without a loader will not participate in middleware. This is problematic for routes that do not have a loader for many valid reasons. One example: I want to continue to use useQuery in a route or don't have time to migrate, but I still want to protect that route with an auth middleware.

In my custom implementation of middleware in dataStrategy, I have to add loader: () => null to routes that I want to "force" into the middleware matching. This is not obvious and is a confusing DX/API. It also creates cruft on a bunch of routes that haven't been converted to use loaders yet.

Should middleware opt a route into the dataStrategy/revalidation?

[sergiodxa]

I agree that middlewares should run regardless if the route has a loader or not, at least of routes behind the umbrella of the the middleware, so if you add a middleware in root every route will be under that middleware, but if you add it to a layout route other routes not nested inside should not trigger it.

[ryan0x44]

Also at a minimum it would be super helpful to have the behaviour clearly documented (with most common use cases like logging and auth as examples), before middleware is deemed 'stable'.

Also would be worth clarifying in the docs that a new context is instantiated on every invocation of the createRequestHandler (which is helpful, for example on Cloudflare Workers, for ensuring context data isn't shared between multiple requests to the same Worker process/isolate from different users). Maybe some kind of diagram would even be helpful for folks. It feels deserving of an entire page on the docs to explain all the aspects of Middleware so that folks can use it correctly and not have to dig through source code to understand how/when it is called and with what context.

[brophdawg11]

lol you gotta give us some time - this comment was 5 days after released a pretty large feature as unstable. We don't always document unstable features up front because we know things will change and we only really want the early adopters that have an appetite for cutting edge stuff to play with it and provide early feedback. Adding it to the docs exposes them to a much wider audience that it's likely not ready for.

not have to dig through source code to understand

Feature, not a bug - those early adopters are willing to do this in order to play with the APIs.

We did get the first pass of docs up a few weeks back as we started to think about stabilization, so definitely let us know if there's feedback there or if we're missing any aspects of the feature 🙌

[brophdawg11]

@infiniteluke It sounds like you're using Data Mode (createBrowserRouter) with a custom dataStrategy? Once you opt-into dataStrategy, you're in charge of executing middleware since you're opting out of the default data logic which includes (which includes both middleware and loaders) but more importantly also includes the logic of opting out of running the data pipeline entirely if no loaders need to be called (which is the root cause of this question).

We provide a little (not yet documented) helper that you can use to run middleware which will run even when no routes have loaders. This should avoid you needing to implement your own middleware logic.

async function dataStrategy({ unstable_runClientMiddleware }) {
  return unstable_runClientMiddleware(async ({ matches }) => {
    const matchesToLoad = matches.filter((m) => m.shouldLoad);
    let results: Record<string, DataStrategyResult> = {};
    await Promise.all(
      matchesToLoad.map(async (match) => {
        results[match.route.id] = await match.resolve();
      })
    );
    return results;
  });
}

See below for a separate comment on the reasoning behind the current logic - I commented here separately to give us a place to discuss the client-side-specific dataStrategy/middleware aspects

[brophdawg11]

Also based on the discussion below we have updated the logic to always run client middlewares (or just "middleware" in data mode) regardless of whether there is loaders to run: #14106

[brophdawg11]

Also reported in #13785.

This is a great question and before we stabilize middleware we'll be sure to call out stuff like this clearly in the docs. I'm going to brain dump my thoughts here a bit on the current design and a jumping off point and we can continue to discuss if we think there ought to be any changes. This is mostly from a framework mode perspective, but generally speaking we try to maintain the same patterns across data/framework to avoid confusion - but there are certainly instances where the logic differs (i.e., Single Fetch).

Middleware is coupled to loader execution to avoid unnecessary server calls when a loader opts out of revalidation and there's no reason to hit the server (this also applies when no loader's exist, but that wasn't necessarily the driving factor for the design). If middleware always runs then it eliminates the ability to optimize your app with shouldRevalidate - specifically those which can result in a navigation that doesn't need to make a .data request.

Consider a root route that has a loader that returns a piece of static data and thus doesn't need revalidation, and also implements a middleware to log request durations:

export const unstable_middleware: Route.unstable_MiddlewareFunction[] = [
  async ({ request }, next) => {
    let start = Date.now();
    let res = await next();
    console.log(request.method, request.url, `(${Date.now() - start}ms)`);
    return res;
  },
];

export function loader() {
  return { version: getAppVersion() };
}

export function shouldRevalidate() {
  return false;
}

If I navigate between two child routes (/a -> /b) and neither have middleware or loaders, then there's no point in making the request because there's no loader data to fetch? It would be pure overhead and a de-optimization (by effectively ignoring shouldRevalidate) to go to the server just to run a middleware to log a no-op request when navigating from /a -> /b?

The same situation would apply if no loader existed on the root route.

The way I think about it is that middleware is coupled to existing data requests made by your app:

1. It can log requests for existing loaders/actions
2. It provides contextual data to downstream loaders (i.e., a shared API client, or session data)
3. It performs user authentication before fetching sensitive data in a downstream loader (which should still be doing authorization checks)

In all of those cases, the middleware is sort of useless without the corresponding loader:

1. There's nothing to log if no loaders need to be fetched
2. There's no need for setting contextual data if nothing needs to access it downstream
3. There's no need to authenticate a user if there's no authenticated actions being performed in the downstream loader

I don't see middleware as wrapping (protecting) views in your app, but rather wrapping/protecting data (or loaders/actions). Maybe said another way - server middleware protects server data. If there is no data to protect, there is no need to run that middleware.

If a route exists, and requires no data, then the only thing that route has is a JS file with the component in it and there's really no need to "protect" that because it's already public via the JS asset. Take the admin.docs.tx route from the reproduction in #13785. The contents of that route are:

export default function Component() {
  return (
    <div>
      <h3>Protected Docs</h3>
      <p>This route should not be visible to unauthenticated users.</p>
    </div>
  );
}

All of which will be publicly available in the JS assets for the route, so middleware doesn't even "protect" the view:

It does provide a small DX enhancement to perform redirects for static routes like this, at the cost of a de-optimization.

The current design allows you to retain the current shouldRevalidate optimizations while giving you a way out by adding a loader. If the design were reversed, you de-opt by default and I don't think there'd be any way to opt-back-into the optimizations?

[brophdawg11]

With that said, I'm certainly open to ideas on a way to approach this that doesn't eliminate the shouldRevalidate optimization.

Maybe there's a case for different behavior when no loaders exist versus when all loaders have opted-out of revalidation - both of which trigger the "I have no loaders to run" short circuit logic? What about when some retained routes opt out and no new routes have loaders? I worry trying to add this type of logic could cause a disproportionate increase in complexity though.

[brophdawg11]

Does this capture the scenarios? The ✅ boxes should be functioning that way today. The top-left box with the ❌ would represent the specific scenario that's broken in the #13785 reproduction and the change we would aim to make. We have no loaders to run, and no existing opted out loaders telling us specifically not to make the call, but middlewares exist on the server so they should run.

	no loaders need to run	some loaders need to run
no loaders have opted out	make the request if middleware exists ❌	make the request ✅
some loaders have opted out	skip the request (due to opt out) ✅	make the request ✅

This would mean that, by default, middleware would always run. As soon as you introduced shouldRevalidate, you'd open up the possibility of opting out of middleware when the new routes in a navigation don't have loaders and all existing route loaders have opted out.

[brophdawg11]

Ignore the above table - we've talked about this a bit more internally and I think we've landed on the proper way to think about this. @jacob-ebey maybe said it most succinctly with "Middleware should not create new network activity".

Just like in express - middleware wraps request handlers. Middleware alone is not a reason for a requests to be made, they only apply to existing requests ("middleware runs when you NEED to hit the server" - @ryanflorence)

The question becomes, what is a "handler" in RR? Is it the route? Or the loader? We think "it depends":

• On document requests (GET /route), the handler encompasses both the loader and the route component/UI - since those are both executed to generate the response
• On data requests (GET /route.data), the handler encompasses only the loader since that's all that is executed for the Response

So we believe middleware should continue to run any time an existing request is made and our current behavior aligns with this:

• Document requests run server middleware whether loaders exist or not because we're still in a "handler" to render the UI
• Client-side navigations will only run server middleware if a .data request is made to the server for a loader

If you want to force a server middleware to run on every client navigation, then you can put it on the root and add an empty loader and it will force a .data request on every navigation which will hit the middleware. We'll make sure this is very clear in the docs.

So from a server middleware perspective, I think the current implementation is accurate. However, I think we should make a change to clientMiddleware. With "middleware runs when you NEED to hit the server" in mind - we're already on the client so I think it makes sense to always run client middlewares on client navs regardless of whether we have loaders to run. I'll look to get this implemented.

[brophdawg11]

#14106 updates the router to always run clientMiddleware even if no loaders exist

[brophdawg11]

#14108 Updates the docs with an explanation around this behavior

[ryanflorence]

middleware depending on if a loader exists or not seems wrong to me. Middleware is a route concern, not a loader concern.

If I want to use a redirects middleware at a route there's no reason to have a loader

Or authentication, even if I don't load any data, maybe there are forms that submit to actions and I want them to login before they hit auth errors there

[brophdawg11]

Doing this would effectively eliminate the behavior of shouldRevalidate, so I think it needs to be more nuanced if we want to keep that around (see #12950 (reply in thread))

[sergiodxa]

Or authentication, even if I don't load any data, maybe there are forms that submit to actions and I want them to login before they hit auth errors there

I initially thought this too, but then I started to think that the middleware should authenticate the user, but the loader should be the one authorizing, so in this case if the user is not authenticated that's an authorization thing and should be done at the loader

This way there's a loader and middleware will run too