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

node callbacks aren't defined quite right #2123

Closed
rmg opened this issue Jul 24, 2016 · 6 comments
Closed

node callbacks aren't defined quite right #2123

rmg opened this issue Jul 24, 2016 · 6 comments

Comments

@rmg
Copy link

rmg commented Jul 24, 2016

The form used for the callbacks right now is generally along the lines of:

type errback = (err: ?Error, result: any) => void;

This works relatively well for most cases, but it breaks down if you write code that actually supports the node "errback" convention you run in to problems. A more accurate way to represent these callbacks would be something more like this:

type errback = {
   (err: Error): void;
   (err: null, result: any): void;
}

Unfortunately, this doesn't seem to work out as well as advertised:

type ErrBack<T> = {
   (err: Error, result: null): void;
   (err: null, result: T): void;
}

declare function someNodeMethod(arg: string, callback: ErrBack<{foo:string}>): void;

someNodeMethod('foo', function(err, res) {
  if (err) {
    // flow knows err is defined because of the conditional
    console.error('failed:', err);
    return;
  }
  // flow knows err is null here because of the branch
  // flow thinks res could be null, but it can't
  console.log('success:', res.foo);
});

It would be nice if flow could treat the pair of arguments as a tagged/disjoint type so that flow could follow the branches and know that the error and success cases are mutually exclusive.

note: I have seen this discussed elsewhere, but can no longer find it.. so the meta-bug is the subject lacks google juice.

@drifkin
Copy link

drifkin commented Aug 15, 2016

I've been looking for an answer to this question as well. I think the older discussion you may be referring to is #166 "Capture complete type of a node-style callback"?

@bwlt
Copy link
Contributor

bwlt commented Mar 2, 2017

Following the intersection example I tried with

type Callback<T> = ((error: Error) => mixed) & ((null, result: T) => mixed)

but is does not work :(

@jfirebaugh
Copy link

I tried:

// @flow

type Callback = (...args: [Error] | [null, string]) => void;

function producer(callback: Callback) {
    if (global.test) {
        callback(new Error('error'));
    } else {
        callback(null, 'result');
    }
}

function consumer() {
    producer((err, result) => {
        if (err) {
            console.log(err);
        } else {
            console.log(result.length);
        }
    })
}

But this still produces:

src/test.js:18
 18:             console.log(result.length);
                                    ^^^^^^ property `length`. Property cannot be accessed on possibly undefined value
 18:             console.log(result.length);
                             ^^^^^^ undefined (too few arguments, expected default/rest parameters)

@vkurchatkin
Copy link
Contributor

It's impossible to do this properly. This is the best solution at the moment:

type Callback = (?Error, ?string) => void;

function producer(callback: Callback) {
    if (global.test) {
        callback(new Error('error'));
    } else {
        callback(null, 'result');
    }
}

function consumer() {
    producer((err, result) => {
        if (err) {
            console.log(err);
        } else if (result) {
            console.log(result.length);
        }
    })
}

@calebmer
Copy link
Contributor

@vkurchatkin is correct. Currently there is not a great way to model this properly. Promises also help make this less of an issue 👍

@kkaefer
Copy link

kkaefer commented Dec 11, 2018

This is still an issue and makes writing flowtype for Node-style callbacks cumbersome.

The intersection example sounds like it should work, but it seems like flow isn't able to correctly match the two distinct signatures and verify them independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants