Abstract return type of callbacks about reason-nodejs HOT 8 CLOSED

@woeps I apologize, but I am not quite sure I understand the problem you are describing. Could you provide some example code to demonstrate the issue?

from reason-nodejs.

Don't apologize! I should have provided an example in the first place.
Let's take ReasonNode.Stream.Transform.makeOptions as an arbitary example.

If you simply change the return type of the transform and flush functions, it's not possible to not call the callback anymore. Like this:

type calledBackTransform;
type calledBackFlush;
[@bs.obj]
  external makeOptions:
    (
      ~objectMode: [@bs.as {json|false|json}] _,
      ~highWaterMark: int=?,
      ~emitClose: bool=?,
      ~autoDestroy: bool=?,
      ~transform: [@bs.this] (
                    (
                      t('w, 'r),
                      'w,
                      string,
                      (~err: option(Js.Exn.t), ~data: option('r)) => calledBackTransform
                    ) =>
                    calledBackTransform
                  ),
      ~flush: [@bs.this] (
                (
                  t('w, 'r),
                  (~err: option(Js.Exn.t), ~data: option('r)) => calledBackFlush
                ) =>
                calledBackFlush
              ),
      unit
    ) =>
    makeOptions('w, 'r);

This is not perfect, because I could "evilish" call makeOptions two or more times and use the callback provided by the other function. In this case the compiler won't complain and it's probably still not the intended usage. But the chances of forgetting the callback ar less than before.

from reason-nodejs.

@woeps What do you mean by "missing callbacks" ? These callbacks are ones you pass to Nodejs, and Node internally calls it - Therefore I don't know what a "missing" callback mean in this scenario.

from reason-nodejs.

I'll try to explain on the example of the ~transform~ function, but the same holds true for ~flush and similar functions in the node api.
The node docs of transform._transform state:

The callback function must be called only when the current chunk is completely consumed.

Currently the type system will evaluate the ~transform parameter successfully if it looks like this (minimal exampe): ~transform=[@bs.this](this, chunk, encoding, callback) => {let transformed_chunk = someOperation(chunk); Js.log(transformed_chunk}
You can see the callback's call is missing, but since the function returns unit the types check.

Now if I introduce an abstract type like in my previous example then this wouldn't typecheck, since unit is not equal to the abstract type. The only way to satisfy the type constraint is to call the callback last: ~transform=[@bs.this](this, chunk, encoding, callback) => {let transformed_chunk = someOperation(chunk); Js.log(transformed_chunk}, callback(~error=None, ~data=transformed_chunk.

from reason-nodejs.

@woeps Understand now, the callback we must call is the one given to us by the transform callback, and we want to encode that invariant into the type signature by using abstract type as the return type of both transform and the callback.

from reason-nodejs.

@woeps Do you think you can send a PR for this feature and see what the diff looks like?

from reason-nodejs.

@sikanhe sure, I will do so for the Stream module since that's what I'm most familiar with by now.

from reason-nodejs.

closed with #29

from reason-nodejs.