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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): http route builder #103

Merged
merged 9 commits into from Feb 15, 2019

Conversation

Projects
2 participants
@JozefFlakus
Copy link
Member

commented Feb 11, 2019

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)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the new behavior?

Introducing the new, EXPERIMENTAL and more functional way of building HTTP effect routes:

The old way of using EffectFactory:

const postUser$ = EffectFactory
  .matchPath('/user')
  .matchType('POST')
  .use(req$ => req$.pipe(
     // ...
  ));

The new way:

const postUser$ = r.pipe(
  r.matchPath('/user'),
  r.matchType('POST'),
  r.useEffect(req$ => req$.pipe(
     // ...
  )));

What does it give us?

  • Consistent API (see RxJS stream pipe)
  • HTTP route definition with functional flavor 馃憠 avoids OOP builder pattern
  • Open API for the future
  • We can apply additional metadata using r.applyMeta function
  • Opens the possibility for using |> operator when the TC39 proposal will reach the desired stage
const postUser$ = r.pipe(
  r.matchPath('/user'),
  r.matchType('POST'),
  r.use(m_1$),
  r.use(m_2$),
  r.use(m_3$),
  r.useEffect(req$ => req$.pipe(
     // ...
  )),
  r.applyMeta({ data: 'some meta here' }),
);

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other

Since version 2.0 both APIs will be valid. It will be up to the developer which way he will choose.
We will see where the new API will bring us and what future possibilities it will open.

Besides the main feature, the PR includes fp-ts library as a core dependency. I see it's more usefullness in the near future.

@JozefFlakus JozefFlakus added this to the 2.0.0-rc.2 milestone Feb 11, 2019

@JozefFlakus JozefFlakus self-assigned this Feb 11, 2019

@JozefFlakus JozefFlakus added this to In progress in @marblejs/next via automation Feb 11, 2019

@codecov

This comment has been minimized.

Copy link

commented Feb 11, 2019

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff         @@
##           next   #103   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files        85     87    +2     
  Lines      1012   1027   +15     
  Branches     89     81    -8     
===================================
+ Hits       1012   1027   +15
Impacted Files Coverage 螖
packages/middleware-io/src/io.reporter.ts 100% <酶> (酶) 猬嗭笍
packages/middleware-jwt/src/jwt.util.ts 100% <100%> (酶) 猬嗭笍
packages/middleware-logger/src/logger.util.ts 100% <100%> (酶) 猬嗭笍
packages/core/src/+internal/utils/string.util.ts 100% <100%> (酶) 猬嗭笍
packages/core/src/router/router.query.factory.ts 100% <100%> (酶) 猬嗭笍
packages/websockets/src/error/ws-error.provider.ts 100% <100%> (酶) 猬嗭笍
packages/core/src/index.ts 100% <100%> (酶) 猬嗭笍
packages/core/src/+internal/utils/index.ts 100% <100%> (酶) 猬嗭笍
packages/core/src/router/router.factory.ts 100% <100%> (酶) 猬嗭笍
packages/core/src/+internal/utils/any.util.ts 100% <100%> (酶)
... and 7 more

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 0f3246d...d86263b. Read the comment docs.

@mistyharsh

This comment has been minimized.

Copy link

commented Feb 12, 2019

This looks great. I am looking at the pipe implementation. The type definition is:

type Pipeable = (route: Either<CoreError, Partial<RouteEffect>>) => Either<CoreError, Partial<RouteEffect>>;

It looks like it won't prevent users from doing this:

const postUser$ = r.pipe(
  r.matchPath('/user'),
  r.matchType('POST'),
  r.use(m_1$),
  r.matchType('POST'),   // PROBLEM
  r.use(m_2$),
  r.useEffect(req$ => req$.pipe(
     // ...
  )));
@JozefFlakus

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

Hi @mistyharsh,

The problem with pipe implementation is that without introducing indexed monads for hamburger-like building (which is big a limitation from TypeScript side), we can't really know which "operator" will be applied first on which position.

The possibilities that I can see for now:

  • to define a limited set of possible operators placement, eg. PathOperator -> TypeOperator -> MiddlewareOperator -> ... -> EffectOperator
  • to allow passing an infinite number of operators (it will remain the same)
  • to experiment with fp-ts IxMonad, but I'm not sure if it will be possible to achieve the desired effect without too many boilerplate code
@JozefFlakus

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

@mistyharsh Yesterday I was playing with indexed monad and it turned out that it can be perfectly applied to this set of problems. I was able to preserve exactly the same pipe API.

The only drawback is that we have to limit the maximum number of operators applied to the pipe chain in order to have the prover type inference across the IxMonad.

const postUser$ = r.pipe(
  r.matchPath('/user'),
  r.matchType('POST'),
  r.use(m_1$),
  r.matchType('POST'),   // the compiler will warn here 馃檪 
  r.use(m_2$),
  r.useEffect(req$ => req$.pipe(
     // ...
  )));

BTW. this kind of problem-solving deserves in the future for some medium article I think. 馃

You can check the implementation details here

@mistyharsh

This comment has been minimized.

Copy link

commented Feb 14, 2019

Yes. That looks great. I was looking at Ramda compose implementation for this issue. Essentially, this compiles very well:

const postUser$ = r.useEffect(
    r.use(m_2$)(
        r.use(m_1$)(
            r.matchType('POST')(
                r.matchPath('/user')
            )
        )
    ),
    req$ => req$.pipe(/* ... */)
);

Its similar representation with compose compiles perfectly with TypeScript. The implementation here resembles the one in Ramda. I think it should be fine to limit pipe/compose to 9-10 operators. The safety provided by this still outweights the benefit of infinite pipeline as we hardly write code that big.

And if it needs to be split we can always go back to nested switchMap. But again, going beyond 10 would probably count as antipattern.

@JozefFlakus JozefFlakus removed the WIP label Feb 14, 2019

@JozefFlakus JozefFlakus force-pushed the feat/http-route-builder branch from e9937cb to d86263b Feb 14, 2019

@JozefFlakus JozefFlakus merged commit 6c9dafb into next Feb 15, 2019

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 In progress to Done Feb 15, 2019

@JozefFlakus JozefFlakus deleted the feat/http-route-builder branch Feb 15, 2019

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