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

Type-safe request extensions #317

Open
sjanota opened this issue Jan 29, 2021 · 7 comments
Open

Type-safe request extensions #317

sjanota opened this issue Jan 29, 2021 · 7 comments
Labels
question Further information is requested scope: core Relates to @marblejs/core package

Comments

@sjanota
Copy link

sjanota commented Jan 29, 2021

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

@JozefFlakus JozefFlakus added question Further information is requested scope: core Relates to @marblejs/core package labels Feb 2, 2021
@JozefFlakus
Copy link
Member

JozefFlakus commented Feb 2, 2021

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. :)

@sjanota
Copy link
Author

sjanota commented Feb 8, 2021

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.

@JozefFlakus
Copy link
Member

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

@sjanota
Copy link
Author

sjanota commented Feb 8, 2021

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

@sjanota
Copy link
Author

sjanota commented Feb 19, 2021

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
Copy link
Author

sjanota commented Feb 19, 2021

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.

@JozefFlakus
Copy link
Member

JozefFlakus commented Feb 20, 2021

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$),  // 👈
    )),
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested scope: core Relates to @marblejs/core package
Projects
None yet
Development

No branches or pull requests

2 participants