Skip to content

Conversation

TrySound
Copy link
Contributor

Summary

According spec all props of subscriber can be omitted. Also I added first test case for subscriber and would like to improve coverage step by step.

Copy link
Member

@briancavalier briancavalier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about Subscriber method variance

// complete value parameter is deprecated
complete(value?: A): void;
complete?: (value?: A) => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had problems in the past where the property style (vs. method style) object type definition prevents class instances from satisfying the type. The reason is that flow requires the properties to be own props, but class methods are inherited (on the prototype).

There's some related info here, here, and here

One way to verify is to create a class that has a next method and see if it type checks as a Subscriber. Something like:

class Test {
  next (a: number) void {}
}

const s: Subscriber<number> = new Test()

If that doesn't work, I think one solution may be to make the properties covariant explicitly:

export type Subscriber<A> = {
  +next?: (value: A) => void;
  +error?: (err: Error) => void;
  // complete value parameter is deprecated
  +complete?: (value?: A) => void;
}

@TrySound
Copy link
Contributor Author

TrySound commented Sep 1, 2017

Done. Covered with test.

Copy link
Member

@briancavalier briancavalier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

next() {};
error() {};
complete() {};
}, just(1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@briancavalier briancavalier merged commit e6b35a7 into cujojs:master Sep 2, 2017
@TrySound TrySound deleted the flow-optional-subscribe-props branch September 18, 2017 05:36
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

Successfully merging this pull request may close these issues.

2 participants