search

RealOrangeOne / react-native-mock

A fully mocked and test-friendly version of react native (maintainers wanted)
MIT License
571 stars 153 forks source link

feat(NativeAppEventEmitter): use fbemitter #87

Closed mroswald closed 5 years ago

mroswald commented 8 years ago

…to React Native's NativeAppEventEmitter

This PR uses the fbemitter instead of node's events since the api is more similar. It could also be used for the NativeEventEmitter (https://github.com/lelandrichardson/react-native-mock/commit/d0ad1d930376805b931051dafe3335de5b6ec390) instead of implementing it yourself

RealOrangeOne commented 8 years ago

I havent used NativeEventEmitter, but if the API is identical to fbevent, could you swap it out in this PR too?

mroswald commented 8 years ago

@RealOrangeOne So it's missing removeListener and removeSubscription on EventEmitter, but that's implemented on NativeEventEmitter, right? Please double-check if it's works that way. For my testcases it worked

I also rebased to current master to get rid of the travis errors

RealOrangeOne commented 8 years ago

I havent looked at EventEmitter until now, but i'm guessing removeListener is rather important. I'm not sure on the relationship between NativeEventEmitter, NativeAppEventEmitter and EventEmitter?

mroswald commented 8 years ago

To be honest, me too. As far as I understood NativeEventEmitter is for subscribing on a native module while NativeAppEventEmitter seems to handle global events? EventEmitter itself is just a base class for handling events and subscriptions.

RealOrangeOne commented 8 years ago

I'm not sure, might pay to get another opinion on this before doing such a large change. @lelandrichardson care to weigh in here?

lelandrichardson commented 7 years ago

hmm. I like how red this PR is. Let me pull this branch locally and make sure it doesn't break anything for us locally. Sorry for the slow response @mroswald !

mroswald commented 7 years ago

@lelandrichardson anything I can help with?