Add support for client context and middleware (unstable)

[brophdawg11]

Add middleware support to React Router behind a future.unstable_middleware flag. This also required a client-side context API for loader/action functions.

See the changeset for example usage.

Middleware RFC: remix-run/remix#7642
Client Context RFC: #9856
Closes: #12695
Experimental release: 0.0.0-experimental-23ff92a9d

[changeset-bot]

🦋 Changeset detected

Latest commit: 979cf6c

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

This PR includes changesets to release 11 packages

Name	Type
react-router	Patch
@react-router/architect	Patch
@react-router/cloudflare	Patch
@react-router/dev	Patch
react-router-dom	Patch
@react-router/express	Patch
@react-router/node	Patch
@react-router/serve	Patch
@react-router/fs-routes	Patch
@react-router/remix-routes-option-adapter	Patch
create-react-router	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

[brophdawg11]

SPA Mode never should have de-optimized revalidation the way single fetch does because there is no server

[brophdawg11]

Because middleware is part of the default data strategy, we have to re-implement it here in our custom data strategy and can do so using the same runMiddlewarePipeline API we use internally. I'm thinking we should make some form of this public API as well for userland dataStrategy implementation who want to use the normal middleware.

The current API is as follows - may be leaking some implementation details we could hide in the exported version though:

runMiddlewarePipeline(
  // Passthrough of { request, matches, context } from dataStrategy
  args, 

  // how deep?  I.e., what is the lowest handler to run
  matchIndexToRunMiddlewareTo, 

  // Should I bubble up a returned Response?  SSR only - always `false` in user client-side implementations
  false, 

  // callback to run the handlers and assign results to keyedResults
  // async (keyedResults: Record<string, DataStrategyResult>) { ... },

  // Error callback if a middleware throws an error - assign the error to keyedResults
  async (e: MiddlewareError, keyedResults: Record<string, DataStrategyResult>) { ... }  
)

Maybe we could pass it as an arg to dataStrategy? We could remove the boolean and handle that for them internally, and then instead of using an index we could just let them hand us the matches which they could .slice if they didn't want to run all the way down:

function dataStrategy({ request, params, context, matches, runMiddleware }) {
  return runMiddleware(
    { request, params, context }, 
    matches,
    (results) => { /* run handlers, assign to results */ },
    (e, results) => { /* handle error */ },
  );    
})

[brophdawg11]

We only run client middleware down to the lowest server loader we will run

[brophdawg11]

We used to use a dataStrategy on the server to filter out matches (for ?_routes support ) and to skip running loaders (for POST /path.data requests) but it was one more layer of abstraction and they felt like useful APIs to have built in anyway so filterMatchesToLoad/skipRevalidation bring that logic into .query so we can get rid of our server side dataStrategy entirely

[brophdawg11]

This is the new API on the static handler to enable middleware. Currently we return a StaticHandlerContext from this method which is of the shape { loaderData, actionData, errors, ... }. But for middleware we need to generate a Response after running the loaders/actions that we can return from next and bubble back up the middleware chain. So we had 2 options - we could implement server side middleware in server-runtime code entirely separate from the staticHandler but that feels inconsistent with the client side implementation and also potentially means a separate set of code.

Instead, if we have the user tell us how to convert StaticHandlerContext -> Response then we can run all the middleware inside .query() and use all the same code and make .query a more useful API on it's own.

[brophdawg11]

Create a new context instance per navigation/fetcher call

[brophdawg11]

If we received an unstable_respond method and one of our routes has middleware, run the new code path. This means that we won't run the new code for anyone not opted into middleware

[brophdawg11]

new skipRevalidation behavior - short circuits after running the action

[brophdawg11]

Run the old code if no middleware exists

[brophdawg11]

This is the main implementation to run the middlewares and provide the next method

[mjackson]

I made a few comments to try and line up types between this work and the work we'll be launching later this year in Remix. It would be nice if we shared the same middleware API between RR and Remix.

[sergiodxa]

This will be a breaking change for many apps, is there no way to keep the AppLoadContext? I know many use cases can be solved by middlewares but being able to inject from the HTTP server into the RR app is useful

[pawelblaszczyk5]

From my initial testing it seems like it'll still be possible - you'll just need to pass/retrieve it a bit differently 😃

[brophdawg11]

Correct - you can use getLoadContext to pass values through from the adapter - it just needs to conform to the new ContextProvider API and return a Map<RouterContext, unknown>: https://github.com/remix-run/react-router/blob/brophdawg11/feat-middleware/playground/middleware/server.ts#L26

[sergiodxa]

ohh in that case there's no issue, I would worry that this is a breaking change tho so should this be marked as v8_middleware once stable?

[rossipedia]

Correct - you can use getLoadContext to pass values through from the adapter - it just needs to conform to the new ContextProvider API and return a Map<RouterContext, unknown>: brophdawg11/feat-middleware/playground/middleware/server.ts#L26

This is going to hurt. A lot. We have hundreds of routes with lots of complex loader and action functions, and thousands of tests, that rely heavily on the current AppLoadContext implementation.

This is 100% a breaking change and should be a v8 flag IMO.

[rossipedia]

However, the type story around AppLoadContext isn't great

Can we get some elaboration on this part? I've been super happy with the type story around AppLoadContext, I'd be curious to see where my experience differs from others.

[brophdawg11]

Yeah - this is a known breaking change and the flag will stabilize as future.v8_middleware once it's ready

[pawelblaszczyk5]

I hope it's helpful here, few issues from making branch with the experimental version at my job project:

1. HandleDocumentRequestFunction is still typed as receiving the old AppLoadContext despite getting proper new context provider
2. This also impacts the new types and makes the stuff really painful, because context isn't unstable_RouterContextProvider - AppLoadContext in typegen files isn't correctly augmented when using NodeNext module resolution #12715
3. Is there some type that can be used to annotate standalone middleware (outside of route files)? There's unstable_MiddlewareFunction, but it forces me to manually pass unstable_RouterContextProvider and Response | void generics to match the type of middleware inside of route
4. Nothing necessary - but it'd be nice if defaultValue on context wasn't optional if it was passed to unstable_createContext

Also thanks a lot for working on that, it's really awesome to use and allowed me to simplify the flow a lot!

[fullstackwebdev]

Is it possible to modify a request, maybe modify next() to accept a new request object?

here's some code demonstration the idea:

  // Clone the request using request.clone()
  const clonedRequest = request.clone();

  // do something to modify cloned request
...
 
  // Pass the cloned request to next middleware/loader
  let response = await next({  /// Allow next to take a clonedRequest so it can be modified
    request: clonedRequest,
    params,
    context
  });

[brophdawg11]

@pawelblaszczyk5 Thanks for the feedback!

1/2 - The context types should be fixed up by 6b61caa.

3 - For now I don't think we'll do a non-Route specific middleware type for initial release because that would mean we don't have type-safe params for those middlewares. You can always just import { Route } from "./+types/root for top-level or otherwise param-agnostic middlewares.

4 - I don't think you should ever be directly accessing defaultValue, so is that optional type messing with your app code?

@fullstackwebdev

It's an interesting idea - probably not something we'll add for initial release but feel free to open up a Proposal Discussion outlining your concrete use cases and we can see if it's a popular ask from the community?

[sergiodxa]

Is it possible to modify a request, maybe modify next() to accept a new request object?

here's some code demonstration the idea:

  // Clone the request using request.clone()
  const clonedRequest = request.clone();

  // do something to modify cloned request
...
 
  // Pass the cloned request to next middleware/loader
  let response = await next({  /// Allow next to take a clonedRequest so it can be modified
    request: clonedRequest,
    params,
    context
  });

what would you need to change in the request that you couldn't use context to pass to the route loader/action?

[pawelblaszczyk5]

Re 2 - I'm not sure if that's fixed with latest changes - the issue is that with NodeNext module resolution, when I extend some interface from library like old AppLoadcontext or new Future the overwritten types aren't correctly applying for stuff imported from the type generated files.

Re 3 - My use case was implementing middleware like authMiddleware in one place and reusing it in various routes that don't share any layout, where I could place such middleware

Re 4 - tbh I don't even recall what was my idea with using defaultValue in app code, so you're probably right and I was doing something wrong 😄

[brophdawg11]

You can put a generic/route-agnostic middleware anywhere in code and then just import it to all the routes that need it. If you don't want to use unstable_MiddlewareFunction and it's generics, you could just use the root route middleware type, which will have no params and be suitable for usage on any route:

// app/middleware/auth.ts
import type { Route } from "../+types/root";

export const authMiddleware: Route.unstable_MiddlewareFunction = async (...) => {...}

I did remove the first generic in a4ed390 because middleware should always be used with the new context type, so it should be simplified to unstable_MiddlewareFunction<Response> now for server middlewares if you want to use that type directly.

[sergiodxa]

My use case was implementing middleware like authMiddleware in one place and reusing it in various routes that don't share any layout, where I could place such middleware

You could create a pathless layout that renders nothing.

export async function unstable_middleware(args: Route.MiddlewareArgs) {
  // code
}

export default function Component() {
  return <Outlet />
}

[pawelblaszczyk5]

My use case was implementing middleware like authMiddleware in one place and reusing it in various routes that don't share any layout, where I could place such middleware

You could create a pathless layout that renders nothing.

export async function unstable_middleware(args: Route.MiddlewareArgs) {
  // code
}

export default function Component() {
  return <Outlet />
}

I don’t think that’d work for all cases - I have some “api/“ routes that all share the same middleware but some of them should also use auth middleware. And the “app/“ routes should all use auth middleware - I think I can’t structure them even with path less layout to make this work 😃

[sergiodxa]

@brophdawg11 does middlewares apply to resource routes?

Right now resource routes are never nested so there's no point of a middleware there, it would be simpler to just call a function inside the resource route loader/action as currently.

[Bricklou]

@brophdawg11 does middlewares apply to resource routes?

Right now resource routes are never nested so there's no point of a middleware there, it would be simpler to just call a function inside the resource route loader/action as currently.

One question i currently having about resource routes and middlewares : what if I have a whole /protected/ route layout that apply a middleware to check the auth state, and then I have a nested resource route /protected/some-actions. My question is the following: will the auth middleware from /protected will apply to /protected/some-actions?

[sergiodxa]

Based on how it works right now, it shouldn't, because the resource route doesn't have parent routes

[brophdawg11]

resource routes are never nested

This isn't a limitation? Resource routes can be nested, and middleware will apply to them just like other nested routes: https://stackblitz.com/edit/github-8b5yasye

[github-actions]

🤖 Hello there,

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