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

Support React Native EventEmitter type with fromEvent #3809

Closed
ajcrites opened this issue Jun 6, 2018 · 3 comments
Closed

Support React Native EventEmitter type with fromEvent #3809

ajcrites opened this issue Jun 6, 2018 · 3 comments
Labels
feature PRs and issues for features

Comments

@ajcrites
Copy link
Contributor

ajcrites commented Jun 6, 2018

Feature Request

React Native defines its own EventEmitter type that it claims is a subset of the Node.js EventEmitter type, but it is a bit different: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-native/index.d.ts#L143

Specifically, removeListener returns void. The return type of addListener and parameters are also different, but this doesn't cause a typing problem.

Is your feature request related to a problem? Please describe.
If you attempt to use fromEvent with a React Native EventEmitter implementation such as https://github.com/urbanairship/react-native-module you will get a type error:

fromEvent(UrbanAirship, 'pushReceived');

Argument of type 'typeof UrbanAirship' is not assignable to parameter of type 'FromEventTarget<{}>'.
  Type 'typeof UrbanAirship' is not assignable to type 'ArrayLike<EventTargetLike<{}>>'.
    Index signature is missing in type 'typeof UrbanAirship'.

Describe the solution you'd like
There are at least two solutions:

  1. Add a ReactNativeEventEmitter type.
  2. Allow addListener and removeListener for NodeStyleEventEmitter to return any.

The isNodeStyleEventEmitter check can be used for both, but unfortunately I don't think there is a way to know whether the provided EventEmitter is Node.js style or React Native except based on type. However, the return types for addListener and removeListener are not used, so being able to distinguish between them at runtime won't matter at this time.

Describe alternatives you've considered
You can of course do fromEvent(UrbanAirship as any... or use fromEventPattern, but these are more verbose and you lose type safety you could get from the former.

@cartant
Copy link
Collaborator

cartant commented Jun 7, 2018

I would like to see the Node-style declarations relaxed, too.

But rather than any, I'd like to see the return values declared as void | {}. As @ajcrites has pointed out, the return values are not used in RxJS, so this should not be a problem.

Did you want to submit a PR for this, Andrew, or do you want me to do it?

@ajcrites
Copy link
Contributor Author

ajcrites commented Jun 7, 2018

I'll be happy to -- other than changing the return type to void | {}, React Native's EventEmitter.addListener handler returns EmitterSubscription, so it is incompatible with NodeEventHandler. Can we change that return type to void | {} as well?

@benlesh benlesh added the feature PRs and issues for features label Jun 7, 2018
@cartant
Copy link
Collaborator

cartant commented Jun 8, 2018

I don't think NodeEventHandler needs to change, but I do think symbol needs to be removed from the first parameter's union type:

interface NodeStyleEventEmitter {
    addListener: (eventName: string | symbol, handler: NodeEventHandler) => this;
    removeListener: (eventName: string | symbol, handler: NodeEventHandler) => this;
}
declare type NodeEventHandler = (...args: any[]) => void;

interface RelaxedNodeStyleEventEmitter {
    addListener: (eventName: string, handler: NodeEventHandler) => void | {};
    removeListener: (eventName: string, handler: NodeEventHandler) => void | {};
}

interface EmitterSubscription {
    context: any;
}
interface EventEmitterListener {
    addListener(eventType: string, listener: (...args: any[]) => any, context?: any): EmitterSubscription;
}
interface EventEmitter extends EventEmitterListener {
    removeListener(eventType: string, listener: (...args: any[]) => any): void;
}

declare const nodeEmitter: NodeStyleEventEmitter;
declare const reactNativeEmitter: EventEmitter;

const e1: NodeStyleEventEmitter = reactNativeEmitter; // Error
const e2: RelaxedNodeStyleEventEmitter = reactNativeEmitter; // OK
const e3: RelaxedNodeStyleEventEmitter = nodeEmitter; // OK

When you submit a PR, could you please add some type tests to make sure that actual Node-style event emitters and React Native event emitters play nice with fromEvent. Just use interface declarations, like those above, within each type test.

See here for an example.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature PRs and issues for features
Projects
None yet
Development

No branches or pull requests

3 participants