DenisFrezzato / hyper-ts

Type safe middleware architecture for HTTP servers
https://denisfrezzato.github.io/hyper-ts/
MIT License
391 stars 18 forks source link

`bindTo` messes up the monadic index #48

Closed kylegoetz closed 3 years ago

kylegoetz commented 3 years ago

🐛 Bug report

bindTo messes up the monadic index. For example, consider a broader monadic index that takes you through AuthenticationOpen to AuthorizationOpen and then StatusOpen and so forth.

authentication middleware:

declare const authenticate: Middleware<AuthenticationOpen, AuthorizationOpen, unknown, AuthenticatedUser>

one of many authorization middlewares

declare const userHasReadAccess: (u:AuthenticatedUser) => Middleware<AuthorizationOpen, StatusOpen, unknown, AuthorizedUser>

const myController = pipe(
  authenticate,
  H.ichain(userHasReadAccess),
  H.bindTo('user'), // <-- property AuthenticationOpen is missing in type StatusOpen . . .
  H.bind('query', () => H.decodeQuery(/* . . . */)),
  /* . . . */

bindTo takes Middleware<I,I,E,A> and returns Middleware<I,I,E,{ ... }> but it should take Middleware<I,O,E,A> because not every middleware passed into bindTo will have the same "in' and "out" index.

Your environment

Which versions of hyper-ts are affected by this issue? Did this work in previous versions of hyper-ts?

Software Version(s)
fp-ts 2.10.5
hyper-ts 0.6.5
TypeScript 3.9.2
kylegoetz commented 3 years ago

Maybe I should have phrased this as: I think this is a bug report. Alternatively, am I misunderstanding how the monadic index is supposed to work here? If it's a bug, I can make a PR. But I want confirmation this is a bug, first. In the meantime, the fix is to avoid bind/bindTo and instead just do nested pipes (which is what we did with fp-ts before bind/bindTo became available.

kylegoetz commented 3 years ago

map should be fixed the same way, right?

Currently, map: (f:(a:A) => B) => (m:Middleware<R,R,E,A>) => Middleware<R,R,E,B> so how would one map over Middleware<I,O,E,A> where I !== O?

Edit and fromPredicate

DenisFrezzato commented 3 years ago

@kylegoetz I've mentioned this is the roadmap for the next version. This is not a bug, more like a feature request for ibind and ibindTo functions. map probably needs to be fixed though, I'll look into that too. Thanks for the report.

kylegoetz commented 3 years ago

@DenisFrezzato can you clarify the difference between bind and ibind then? I never understood the chain and ichain distinction (honestly I only ever use ichain and never chain), except that one does not allow you to change the index, but the other does. Is this the only difference? And thus the only difference between bind and a potential ibind is that with ibind the incoming and outgoing indices can be different? (Sort of like how some w-suffix functions allow a widening of the error type?)

kylegoetz commented 3 years ago

Also, I'll do a PR for map. I've got a PR for I think adding sendFile so it ain't my first rodeo :)

DenisFrezzato commented 3 years ago

can you clarify the difference between bind and ibind then?

It's the same difference between chain and ichain (since bind is just a utility around chain). ichain is the indexed version of chain, which additionally updates the indexes.

honestly I only ever use ichain and never chain

You should use chain when the operation doesn't change the indexes.

import * as H from 'hyper-ts'
import * as M from 'hyper-ts/lib/Middleware'
import { pipe } from 'fp-ts/function'
import * as t from 'io-ts'

declare function sendStatus<E>(
  status: H.Status,
): M.Middleware<H.StatusOpen, H.ResponseEnded, E, void>

const ReqBody = t.type({ name: t.string })

const userHandler = pipe(
  M.decodeBody(ReqBody.decode),
  // Chain a new Middleware, same indexes.
  M.chain((user) => user.name === 'Denis' ? M.right(user) : M.left('Wrong name')),
  // Chain a new Middleware with different indexes.
  M.ichain(() => sendStatus(H.Status.NoContent)),
  M.orElse(() => sendStatus(H.Status.BadRequest))
)

Also, I'll do a PR for map.

map is fine, we need an indexed version imap for that too, see https://hackage.haskell.org/package/indexed-0.1.3/docs/Data-Functor-Indexed.html or https://pursuit.purescript.org/packages/purescript-indexed-monad.

kylegoetz commented 3 years ago

@DenisFrezzato I'm closing my issue since it appears you've implemented this in a forthcoming version.