search

appfeel / node-pushnotifications

Push notifications for GCM, APNS, MPNS, AMZ (automatic detection from device token)
MIT License
544 stars 125 forks source link

update fcm conversion branch and address PR feedback #205

Closed ehaynes99 closed 6 months ago

ehaynes99 commented 6 months ago

This is a fork of: https://github.com/banshiAnton/node-pushnotifications/tree/banshiAnton-fcm-method

That branch has an open PR here: https://github.com/appfeel/node-pushnotifications/pull/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: https://github.com/appfeel/node-pushnotifications/pull/194#issuecomment-2088770323

NicolasBonduel commented 6 months ago

Could you add @alex-friedl as a reviewer?

ehaynes99 commented 6 months ago

Actually, sorry, I thought the original PR was in a better state, but we've found several issues. Also, this is really suspicious: https://github.com/appfeel/node-pushnotifications/blob/2f6c2685f343816cb93edb573e828609e355d3ef/src/sendFCM.js#L83

I'm not comfortable submitting this anymore. I'm going to convert our app to use firebase-admin directly. I'll leave this branch around for a bit if anyone wants to fork it.

Hossman333 commented 6 months ago

@jamesbluecrow or @NicolasBonduel Any clarification on the concern @ehaynes99 brought up? 🤔 👀

jamesbluecrow commented 6 months ago

@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 firebase-admin-sdk instead.