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

imp(core, middleware-joi, @integration): improved type-inference of combined middlewares #88

Merged
merged 4 commits into from Dec 13, 2018

Conversation

Projects
2 participants
@JozefFlakus
Copy link
Member

commented Dec 12, 2018

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[x] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

  • @marblejs/core - use operator can't properly infer combined, chained middlewares, like:
const m1$: Middleware = req$ =>
  req$.pipe(
    tap(req => req.body = { test: 'test' }),
  );

const m2$: Middleware = req$ =>
  req$.pipe(
    tap(req => req.params = { test: true }),
  );

// then

const effect$: Effect = req$ =>
  req$.pipe(
    use(m1$),
    use(m2$),
    tap(req => req.body.test),    // body.test === "any"
    tap(req => req.params.test),  // params.test === "any"
    // ...
  );

Issue Number: #78

What is the new behavior?

  • @marblejs/core - from now use operator will be able to infer combined middlewares by merging two or more consecutive middlewares (the type inference and merging should be done in middleware definition) , eg.
const m1$ = <T extends HttpRequest>(req$: Observable<T>) =>
    req$.pipe(
      tap(req => req.body = { test: 'test' }),
    ) as  Observable<HttpRequest<{ test: string }, T['params'], T['query']>>;

const m2$ = <T extends HttpRequest>(req$: Observable<T>) =>
    req$.pipe(
      tap(req => req.params = { test: true }),
    ) as Observable<HttpRequest<T['body'], { test: boolean }, T['query']>>;

const effect$: Effect = req$ =>
  req$.pipe(
    use(m1$),
    use(m2$),
    tap(req => req.body.test),     // body.test === "string"
    tap(req => req.params.test),   // params.test === "boolean"
    // ...
  );

As you can see the new improved middleware type declaration requires to omit Middleware type alias in place of direct function type declaration for incoming and outgoing parameters. The <_ extends HttpRequest> thing is required by TS to trigger type inference.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

In order to achieve improved type inference (which in general is one of our main goals for 2.0 release), the HttpReqest.body, HttpReqest.params, HttpReqest.query have to be of unknown type by default. This way forces developers to use validation middlewares like eg. @marblejs/middleware-joi, which makes our Effects more bulletproof by validating incoming HttpRequest parameters.

@JozefFlakus JozefFlakus added this to the 2.0.0 milestone Dec 12, 2018

@JozefFlakus JozefFlakus self-assigned this Dec 12, 2018

@JozefFlakus JozefFlakus added this to In progress in @marblejs/next via automation Dec 12, 2018

@codecov

This comment has been minimized.

Copy link

commented Dec 12, 2018

Codecov Report

Merging #88 into next will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##           next    #88   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files        45     44    -1     
  Lines       499    498    -1     
  Branches     62     62           
===================================
- Hits        499    498    -1
Impacted Files Coverage Δ
packages/core/src/http.interface.ts 100% <ø> (ø) ⬆️
...ackages/middleware-joi/src/validator.middleware.ts 100% <100%> (ø) ⬆️
packages/core/src/operators/use/use.operator.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e15ea78...58a0045. Read the comment docs.

@JozefFlakus JozefFlakus moved this from In progress to Needs review in @marblejs/next Dec 12, 2018

@JozefFlakus JozefFlakus merged commit 4cef1f0 into next Dec 13, 2018

2 checks passed

Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details

@marblejs/next automation moved this from Needs review to Done Dec 13, 2018

@JozefFlakus JozefFlakus deleted the imp/next/core/ImprovedTypeInference branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.