fix(remix): Paramaterize server side transactions#5491
Conversation
lforst
left a comment
There was a problem hiding this comment.
Changes seem sound to me! 👍
(I left some comments even though this is a draft but just so we don't forget)
| matchRoutes: (routes: ServerRoute[], pathname: string) => RouteMatch<ServerRoute>[] | null; | ||
| }>('react-router-dom'); | ||
| // https://github.com/remix-run/remix/blob/38e127b1d97485900b9c220d93503de0deb1fc81/packages/remix-server-runtime/routeMatching.ts#L12-L24 | ||
| function matchServerRoutes(routes: ServerRoute[], pathname: string): RouteMatch<ServerRoute>[] | null { |
There was a problem hiding this comment.
Would it be posssible to pull loadModule and matchServerRoutes out of the createRoutes closure?
There was a problem hiding this comment.
Yes I can do this!
| } | ||
|
|
||
| return matches.map(match => ({ | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access |
There was a problem hiding this comment.
I think these eslint disables are not necessary
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| pathname: match.pathname, | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| route: match.route as unknown as ServerRoute, |
There was a problem hiding this comment.
Is this type cast needed? I think the correct type is already inferred.
| const requestHandler = origCreateRequestHandler.call(this, { ...build, routes, entry: wrappedEntry }, mode); | ||
|
|
||
| return wrapRequestHandler(requestHandler); | ||
| return wrapRequestHandler(requestHandler, build); |
There was a problem hiding this comment.
This is working, but just in case anything changes internally in @remix/server-runtime (like comparing the build or route objects or anything), could we use the same build object we provide to origCreateRequestHandler here?
There was a problem hiding this comment.
Good idea, will do
84c5570 to
ea1c5e0
Compare
This makes sure server side transactions are parameterized correctly in Remix.
We do this by matching the server routes the same way Remix does under the hood, by reconciling the routes and URL pathname - https://github.com/remix-run/remix/blob/97999d02493e8114c39d48b76944069d58526e8d/packages/remix-server-runtime/server.ts#L36
Copied over a bunch of code from Remix to make this happen (and attributed accordingly).