feat: use react-native NativeEventEmitter #189

hoangvvo · 2022-04-21T07:09:01Z

This makes use directly of react-native NativeEventEmitter (https://reactnative.dev/docs/native-modules-android#sending-events-to-javascript) and remove react-native-events. Thus, the library is now dependency-free and also avoids the trouble of autolinking involving react-native-events 🎉

Implementation status

Android: I have tested this on Android, and it works alright!

IOS: I do not know how to program c/ios nor have a Mac to test out the changes, but I tried my best to write the c code. It would be great if you can assist me on that. See https://reactnative.dev/docs/native-modules-ios#sending-events-to-javascript for references.

Breaking changes

However, since NativeEventEmitter signature is not the same as our previous event emitter, I have decided to make the breaking change to move the event emitter to a property called events. This is because our native module contains the native method addListener, which would break if I try to attach the methods directly on the object.

The latest commit succeeded in maintaining the event emitter API. I have not added back some of the APIs that are not commonly used (See https://github.com/cjam/react-native-spotify-remote/pull/189/files#diff-2cdc799d0278758d580b911374652f261c50cb134076f329d0ab0b406bcd2623), but we can do it if we want to.

this is already achieved

cjam · 2022-04-23T04:04:48Z

Wow, just saw this!
Thanks for contribution! I'll review this PR as soon as I can.

Dependency free! Would love to see if we could avoid the breaking change, but obviously can if needed.

ios/RNSpotifyRemoteAppRemote.m

android/src/main/java/com/reactlibrary/RNSpotifyRemoteAppModule.java

src/SpotifyRemote.ts

Since it will be transpiled again by Metro anyway

cjam · 2022-05-24T12:02:45Z

src/TypedEventEmitter.ts

 */
 export default interface TypedEventEmitter<T> {
-    on<K extends keyof T>(name: K, listener: (v: T[K]) => void): this;
    addListener<K extends keyof T>(event: K, listener: (v: T[K]) => void): this;


I think where appropriate we can use the new interface. So here, we could probably return the EmitterSubscription. It is a breaking change, but aligns with the NativeEventEmitter implementation better

Done b6abe5b

cjam · 2022-05-24T12:18:16Z

src/SpotifyRemote.ts

+    },
+    removeListener(eventType, listener) {
+        eventListeners[eventType].forEach((eventListener) => {
+            if (!listener || eventListener.listener === listener) {


I think we only need to pass the listener here. And if no listener gets passed in, we just do nothing rather than unsubscribing all existing listeners.

Unsubscribe everything in case no listener is a pattern employed in many event emitter libs. It also helps our other method unsubscribeAllListeners.

b6abe5b should also solve this. we no longer allow passing in no function

cjam · 2022-05-24T12:19:41Z

src/SpotifyRemote.ts

+            nativeModule.eventStartObserving(eventType);
+        }
+        eventListeners[eventType].add(sub);
+        return this;


If we return the sub here. Then this method looks exactly like the nativeEventEmitter and we can add deprecations to removeListener below similar to here

https://github.com/facebook/react-native/blob/2596b2f6954362d2cd34a1be870810ab90cbb916/Libraries/vendor/emitter/_EventEmitter.js#L81-L92

Done via b6abe5b

cjam · 2022-05-24T12:23:13Z

src/SpotifyRemote.ts

+const eventListeners: Record<
+    keyof SpotifyRemoteEvents,
+    Set<EmitterSubscription>
+> = {
+    playerContextChanged: new Set(),
+    playerStateChanged: new Set(),
+    remoteConnected: new Set(),
+    remoteDisconnected: new Set(),
+};


cjam · 2022-06-04T12:55:54Z

@hoangvvo Thanks again for this PR. Looks like a recent update I made introduced a few conflicts with the readme and the docs. Feel free to take the incoming changes and I'll update the contributors stuff after this PR is finished.

Also if you could add an entry to the Unreleased section of the CHANGELOG.md as part of this PR that would be great.

cjam

This looks great, thanks for putting in those changes.
I think you may need to rebase as there was one small PR around the build.gradle version.

Thanks for all your work on this.
I'll have to double check that everything builds on iOS, but might take care of that once this get's merged as sometimes I find it awkward to participate in external PR's.

@hoangvvo if you're interested in contributing more in the future, I can add you as a contributor that would allow you to create branches / PR's directly in this Repo instead of from your fork.

hoangvvo · 2022-06-13T17:07:05Z

@hoangvvo if you're interested in contributing more in the future, I can add you as a contributor that would allow you to create branches / PR's directly in this Repo instead of from your fork.

That would be great. I am actively using this library in some of my side projects and would not mind maintaining it as well.

cjam · 2022-06-17T14:54:46Z

@hoangvvo this has been released in @next (1.0.0-1)

* upstream/master: gradle flatDir to sourceSets (cjam#205) chore: release 1.0.0-1 Fix iOS Build (cjam#202) feat: use react-native NativeEventEmitter (cjam#189) Update gradle to direct include aar files (cjam#199) chore: release 0.3.11-6 Update contributors and changelog Add contribute script and Pull request template Add a null check when mapping tracks (cjam#192)

* Use NativeEventEmitter * Adopt react-native event emitter signature * Update docs

hoangvvo added 2 commits April 21, 2022 13:58

Use NativeEventEmitter

dab0313

Update all-contributors

e2a1bb1

hoangvvo changed the title ~~Use react-native NativeEventEmitter~~ feat: use react-native NativeEventEmitter Apr 21, 2022

This comment was marked as outdated.

Sign in to view

remove comment

16ac308

this is already achieved

cjam reviewed Apr 25, 2022

View reviewed changes

ios/RNSpotifyRemoteAppRemote.m Outdated Show resolved Hide resolved

cjam reviewed Apr 25, 2022

View reviewed changes

android/src/main/java/com/reactlibrary/RNSpotifyRemoteAppModule.java Show resolved Hide resolved

hoangvvo added 3 commits April 26, 2022 00:51

ios: implement supportedEvents

d1d7a43

android: add lazy subscription

bf5acf6

android: fix subscriptionHasListeners map

26093c4

cjam reviewed Apr 29, 2022

View reviewed changes

android/src/main/java/com/reactlibrary/RNSpotifyRemoteAppModule.java Outdated Show resolved Hide resolved

cjam reviewed Apr 29, 2022

View reviewed changes

src/SpotifyRemote.ts Outdated Show resolved Hide resolved

hoangvvo added 2 commits May 22, 2022 03:04

android: use this.reactContext and update function signature

0874856

ios: subscribe to all events if there is at least one listeners

f9d67d0

hoangvvo force-pushed the rn-eventemitter branch from a5cb209 to f9d67d0 Compare May 21, 2022 20:04

hoangvvo added 3 commits May 22, 2022 04:38

Add SpotifyRemote wrapper and restore old event API

fbc6b4a

ios/android: Add lazy event subscription

e55d6f2

example: revert api changes

017aeaa

cjam reviewed May 23, 2022

View reviewed changes

src/SpotifyRemote.ts Show resolved Hide resolved

cjam reviewed May 23, 2022

View reviewed changes

src/SpotifyRemote.ts Outdated Show resolved Hide resolved

cjam reviewed May 23, 2022

View reviewed changes

src/SpotifyRemote.ts Outdated Show resolved Hide resolved

cjam reviewed May 23, 2022

View reviewed changes

src/SpotifyRemote.ts Outdated Show resolved Hide resolved

Refactor event listener code

da73a4f

hoangvvo requested a review from cjam May 24, 2022 08:21

hoangvvo added 4 commits May 24, 2022 15:25

Update docs

4cea50f

Use this for setPlaying function

72acbf1

Fix typo

76d54ec

Build for modern ES

031a32f

Since it will be transpiled again by Metro anyway

cjam reviewed May 24, 2022

View reviewed changes

hoangvvo and others added 3 commits June 11, 2022 18:16

Merge branch 'master' into rn-eventemitter

9ba8f02

Update changelog

179c618

Adopt react-native event emitter signature

b6abe5b

hoangvvo requested a review from cjam June 11, 2022 11:35

Update docs

8f8eeb2

cjam approved these changes Jun 13, 2022

View reviewed changes

Merge branch 'master' into rn-eventemitter

828f821

cjam merged commit 1f99933 into cjam:master Jun 13, 2022

theshadowagent pushed a commit to theshadowagent/react-native-spotify-remote that referenced this pull request Jan 24, 2023

feat: use react-native NativeEventEmitter (cjam#189)

8015298

* Use NativeEventEmitter * Adopt react-native event emitter signature * Update docs

feat: use react-native NativeEventEmitter #189

feat: use react-native NativeEventEmitter #189

Uh oh!

Conversation

hoangvvo commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation status

Breaking changes

Uh oh!

This comment was marked as outdated.

cjam commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjam May 24, 2022

Choose a reason for hiding this comment

Uh oh!

hoangvvo Jun 11, 2022

Choose a reason for hiding this comment

Uh oh!

cjam May 24, 2022

Choose a reason for hiding this comment

Uh oh!

hoangvvo May 24, 2022

Choose a reason for hiding this comment

Uh oh!

hoangvvo Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjam May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoangvvo Jun 11, 2022

Choose a reason for hiding this comment

Uh oh!

cjam May 24, 2022

Choose a reason for hiding this comment

Uh oh!

cjam commented Jun 4, 2022

Uh oh!

cjam left a comment

Choose a reason for hiding this comment

Uh oh!

hoangvvo commented Jun 13, 2022

Uh oh!

cjam commented Jun 17, 2022

Uh oh!

Uh oh!

hoangvvo commented Apr 21, 2022 •

edited

Loading

cjam commented Apr 23, 2022 •

edited

Loading

hoangvvo Jun 11, 2022 •

edited

Loading

cjam May 24, 2022 •

edited

Loading