Stream write callback doesn't allow handling errors

[wclr]

foreign import writeStringImpl
  :: forall r
   . Writable r
  -> String
  -> String
  -> Effect Unit -- callback should allow error handling
  -> Effect Boolean

The callback should allow error handling, for example:
(Nullable Error) -> Effect Unit though the error value passed to the callback may be undefined or js Error.

[JordanMartinez]

See #41 (comment)

[JordanMartinez]

@wclr Can you provide a Node.js example that demonstrates a time where the arg will be undefined? The docs don't really clarify this, so I assumed it was always Error and forgot about the null possibility.

@thomashoneyman I think we'll need to fix this before 0.15.0 is released. Our FFI bindings cannot have wrong type signatures.

[wclr]

Can you provide a Node.js example that demonstrates a time where the arg will be undefined?

It is undefined in v16.13.2:

const fs = require("fs")
var writeStream = fs.createWriteStream("./file.txt")
writeStream.write("some", null, (err) => {
  console.log("err", err) // prints out `undefined`
  writeStream.end((endErr) => {
       console.log("endErr", endErr) // prints out `undefined`
   })
})  

So to use Nullable that means that the implementation:

export function write(w) {
  return chunk => done => () => w.write(chunk, null, done);
}

Should be changed to:

export function write(w) {
  return chunk => done => () => w.write(chunk, null, (err) => done(err || null));
}

The same for writeStringImpl and end (this too may give the error, for example if try to end already finished stream) foreing implementations.

Other approach instread of Nullable use two callbacks (success and onError), though this kind of different api.

[JordanMartinez]

Thanks. I can confirm that. Idiomatically, it would be better if the API exposed to the end-user was Maybe Error -> Effect Unit.

[wclr]

It would be better if the API exposed to the end-user was Maybe Error -> Effect Unit.

I think this is right, I just usually try to make FFI code as simple as possible.

[JordanMartinez]

My thought was that if we used Nullable Error, all end users would have to write something like this:

-- We want to handle both cases
\err -> do
  case toMaybe err of
    Nothing -> ...
    Just e -> ...

-- We only care if it's an actual error:
\err -> for_ (toMaybe err) \actualErr -> ...

-- We only care if it's not an error:
\err -> when (null == err) $ ...

Since 2/3 use toMaybe and the third could be written in terms of Maybe (e.g. when (isNothing err) $ ...), we could implement the FFI like this:

foreign import writeStringImpl
  :: forall r. Writable r -> String -> String -> (Nullable Error -> Effect Unit) -> Effect Boolean

writeString
  :: forall r. Writable r -> String -> String -> (Maybe Error -> Effect Unit) -> Effect Boolean
writeString w s1 s2 cb = writeStringImpl w s1 s2 (cb <<< toMaybe)

[JordanMartinez]

Should be changed to

export function write(w) {
 return chunk => done => () => w.write(chunk, null, (err) => done(err || null));
}

Not quite. The err || null part isn't needed. toMaybe is defined as:

toMaybe :: forall a. Nullable a -> Maybe a
toMaybe n = runFn3 nullable n Nothing Just

nullable is implemented as:

export function nullable(a, r, f) {
  return a == null ? r : f(a);
}

and running nullable(undefined, true, () => false) returns true.

If nullable was implemented as a === null rather than a == null, then we'd need the err || null part.