marble icon indicating copy to clipboard operation
marble copied to clipboard

Type-safe request extensions

Open sjanota opened this issue 4 years ago • 7 comments

Is your feature request related to a problem? Please describe. Right now HttpRequest is can be assigned any property. Thanks to that you can pass custom properties down the stream as middleware-jwt does.

However, custom properties are of type any. I can't see a way to make them type-safe. Am I missing something?

Describe the solution you'd like I'd like to pass custom properties in a type-safe manner as it is done with params, query, and body.

Describe alternatives you've considered I tried to extend the HttpRequest type and make my middleware return the extended version, but with no success.

Additional context

sjanota avatar Jan 29 '21 13:01 sjanota

Hi @sjanota! Could you describe the use case for this problem? Marble.js has many possible ways for composing middlewares and some of them don't infer/pass the type by design. In order to help you, as a first step, I would like to narrow the use case that you are referring to. :)

JozefFlakus avatar Feb 02 '21 11:02 JozefFlakus

I was trying to write a simple authentication middleware, similar to JWT. I was able to write to middleware without a problem but I was stuck on how to safely pass the user data down the stream. I've noticed that in JWT middleware user is assigned to request and thus is considered any in a handler. I was looking for a more type-safe way, so the handler can somehow expect this field to be of a concrete type.

One thing I was trying to do was:

type MyHttpRequest = HttpRequest & {
   user: User
}

Then my middleware was of type Observable<HttpRequest> => Observable<MyHttpRequest> and handler Observable< MyHttpRequest> => Observable<HttpResponse>. I wasn't able to get that working though.

sjanota avatar Feb 08 '21 08:02 sjanota

One additional question - did you try to compose it directly inside the effect? see here

JozefFlakus avatar Feb 08 '21 10:02 JozefFlakus

No, I made it a global middleware. I'll play with the Effect approach later today.

sjanota avatar Feb 08 '21 12:02 sjanota

Took me a while, but I'm back on this. So, I've declared my custom request and middleware like this:

type AuthedHttpRequest = HttpRequest & {
    session: Session
}

const authenticate$: HttpMiddlewareEffect<HttpRequest, AuthedHttpRequest>

Then in an effect, I add it using r.use:

const setUserProperties$ = r.pipe(
    r.matchPath('/:id'),
    r.matchType('PUT'),
    r.use(authenticate$),
    r.useEffect((req$) =>
        req$.pipe(
            ...
        )
    ),
);

Here r.useEffect doesn't resolve req$ as Observable<AuthedHttpRequest>.

One of the reasons is that r.use and r.useEffect don't expose type variables for for HttpMiddlewareEffect and HttpEffect respectively. When I changed their declarations to:

  • use: <I extends HttpRequest = HttpRequest, O extends HttpRequest = HttpRequest>(middleware: HttpMiddlewareEffect<I, O>) => ...
  • useEffect: <I = HttpRequest, O = HttpEffectResponse>(effect: HttpEffect<I, O>) => ...

the compiler properly resolves type of r.use(authenticate$) but r.useEffect still provides Observable<HttpRequest> to the callback.

It works if I specify the type explicitly though: r.useEffect<AuthedHttpRequest>(...). What's interesting, it type-checks even if I don't include my middleware 🤷. There must be something with the definition of pipe.

Also, I've found that reqesutValidator$ can break stuff because it accepts anything that extends HttpRequest but returns HttpRequest, and information about the initial type is lost.

sjanota avatar Feb 19 '21 19:02 sjanota

I pressed "Comment" one test too early :D.

I noticed that both use and useEffect return IxBuilder parametrized with HttpRequest as the last parameter. After I changed that to the types they are parametrized with it type-checks as expected. I'm talking of course about the declaration file in the npm bundle, not the source code.

Looks like RouteEffect could be defined as follows:

interface RouteEffect<I extends HttpRequest = HttpRequest, O extends HttpRequest = HttpRequest> {
  path: string;
  method: HttpMethod;
  effect: HttpEffect<O, HttpEffectResponse>;
  middleware?: HttpMiddlewareEffect<I, O>;
  meta?: RouteMeta;
}

And RouteEffectSpec should expose those types too so use and useEffect can use it. It's doable, I can even prepare a PR if you find it feasible.

However, it would be great if one could achieve the same thing with combineRoutes. I think it's impossible without changing the API, as RouteCombinerConfig accepts an array of middlewares.

sjanota avatar Feb 19 '21 19:02 sjanota

I have to analyze this topic deeper but the problem that you described is already known. For now r.use doesn't infer types from middlewares but you can achieve the same thing using a similar approach:

const setUserProperties$ = r.pipe(
    r.matchPath('/:id'),
    r.matchType('PUT'),
    r.useEffect(req$ => req$.pipe(
      use(authenticate$),  // 👈
    )),
);

JozefFlakus avatar Feb 20 '21 19:02 JozefFlakus