update fcm conversion branch and address PR feedback #205
Conversation
There was a problem hiding this comment.
Hey @ehaynes99 thanks a lot for the effort!
Really appreciate that you take @banshiAnton work and you move it one step forward.
Just a quick question about a file that seems private info in a file, is this correct?
There was a problem hiding this comment.
I'm not sure if this is private info?
There was a problem hiding this comment.
Oh, I'm not sure either. It was in the original fork, added in d331836
| expect(firebaseMessage.android.priority).to.equal('high'); | ||
| expect(firebaseMessage.android.notification).to.deep.include(messageData); | ||
|
|
||
| expect(firebaseMessage.apns.payload.aps.sound).to.equal( |
There was a problem hiding this comment.
Should this be firebaseMessage.android.payload.aps.sound?
There was a problem hiding this comment.
Not according to the types: https://github.com/firebase/firebase-admin-node/blob/a37eb6cb5ad92d911d0b868f5beb8d11761bcc0a/src/messaging/messaging-api.ts#L20
the android?: AndroidConfig does not have a payload property, but apns?: ApnsConfig; does, which has a aps: Aps; that contains a sound?: string | CriticalSound; property.
| messageData.sound | ||
| ); | ||
|
|
||
| expect(firebaseMessage.apns.payload.aps.alert).to.deep.include({ |
There was a problem hiding this comment.
Should this be firebaseMessage.android.payload.aps.alert?
There was a problem hiding this comment.
Not according to the types: https://github.com/firebase/firebase-admin-node/blob/a37eb6cb5ad92d911d0b868f5beb8d11761bcc0a/src/messaging/messaging-api.ts#L20
the android?: AndroidConfig does not have a payload property, but apns?: ApnsConfig; does, which has a aps: Aps; that contains a alert?: string | ApsAlert; property.
|
Could you add @alex-friedl as a reviewer? |
|
Actually, sorry, I thought the original PR was in a better state, but we've found several issues. Also, this is really suspicious: node-pushnotifications/src/sendFCM.js Line 83 in 2f6c268 I'm not comfortable submitting this anymore. I'm going to convert our app to use |
|
@jamesbluecrow or @NicolasBonduel Any clarification on the concern @ehaynes99 brought up? 🤔 👀 |
Nope, I agree with @ehaynes99 (we work together). We tested the PR and it didn't work as expected and we found a couple of bugs. We decided to just move out of this library and use |
This is a fork of:
https://github.com/banshiAnton/node-pushnotifications/tree/banshiAnton-fcm-method
That branch has an open PR here: #194 The original author has not responded in months, and this conversion is necessary before the removal of the legacy firebase API in June.
I don't take credit for any of the prior work. I simply merged the current master and addressed the outstanding PR feedback here: #194 (comment)