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

Add route middleware functionality #9975

Closed
wants to merge 30 commits into from
Closed

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Jan 24, 2023

Middleware implementation to run before/after loaders/actions. API is inspired by the Fresh middleware with the .next() call in the middle thus allowing us to alter responses on the way back up the chain.

Data is passed down through strongly typed get/set methods that operate in a React Context-like manner where you define the typed "context" up front with an optional default value and use that as the key.

Proposal: #9564

  • Middleware pre execution
  • Middleware post execution
  • Strongly typed context
  • future.unstable_middleware flag
  • Rename middleware param to context behind the flag

@changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2023

🦋 Changeset detected

Latest commit: 1e8a428

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

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Minor
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native 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

defaultValue: T
): MiddlewareContextInstance<T> {
return new MiddlewareContextInstance<T>(defaultValue);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcattori Definitely want to get your eyes on this before merging - this all "works" but no clue if it's "right" ;)

@brophdawg11 brophdawg11 changed the title WIP: Middleware Middleware Jan 27, 2023
@brophdawg11 brophdawg11 changed the title Middleware Add route middleware functionality Jan 27, 2023
@brophdawg11 brophdawg11 marked this pull request as ready for review January 27, 2023 16:39
*/
export interface FutureConfig {
unstable_middleware: boolean;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New future config accepted in createRouter

export interface StaticHandlerInit {
basename?: string;
future?: FutureConfig;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future also accepted in createStaticHandler

context: requestContext || disabledMiddlewareContext,
});

result = await Promise.race([dataPromise, abortPromise]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If enabled, call the middleware pipeline, otherwise just call the handler

Comment on lines 3100 to 3118
// Avoid memory leaks since we don't control the key
let store = new WeakMap();
let middlewareContext: MiddlewareContext = {
get<T>(k: MiddlewareContextInstance<T>) {
if (store.has(k)) {
return store.get(k) as T;
}
return k.getDefaultValue();
},
set<T>(k: MiddlewareContextInstance<T>, v: T) {
if (typeof v === "undefined") {
throw new Error(
"You cannot set an undefined value in the middleware context"
);
}
store.set(k, v);
},
next: () => {},
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create the context for this pipeline (scoped to a given loader/action execution)

Comment on lines 3165 to 3365
let next: MiddlewareContext["next"] = () => {
nextCalled = true;
return callRouteSubPipeline(
request,
matches.slice(1),
params,
requestContext,
middlewareContext,
handler
);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide a next() function to this middleware which will launch all child middlewares and the eventual handler

Comment on lines +3189 to +3381
if (nextCalled) {
return res;
} else {
return next();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the middleware resolves without calling next() - just keep going down the chain

@appden
Copy link
Contributor

appden commented Feb 28, 2023

It looks like middleware will get run for document requests, so I think it would be nice to include middlewareContext in StaticHandlerContext for the benefit of accessing it from document handlers if needed.


[pickingarouter]: ../routers/picking-a-router
[react-context]: https://reactjs.org/docs/context.html
[fresh-middleware]: https://fresh.deno.dev/docs/concepts/middleware
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you would middleware composition look like with this new API?
For example, what If I needed to apply 3-4 middleware to my request, which is not an uncommon thing if you're coming from Express or Fastify or in general a production nodejs server 😄

Recently stumbled upon this lib which takes a fetch API first approach & feels very vanilla, thought i'd share 👀
https://github.com/data-eden/data-eden/tree/main/packages/network

CleanShot 2023-02-28 at 04 01 19@2x

@brophdawg11
Copy link
Contributor Author

Closing temporarily - we'll be revisiting shortly after v2 releases

@kstratis
Copy link

Now that v2 has been released, perhaps this should be taken out of the fridge..?

@brophdawg11
Copy link
Contributor Author

It has - see the roadmap

@kstratis
Copy link

Wasn't aware! Thank u!

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.

None yet

6 participants