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

Deprecated FCM APIs, add FCM method (HTTP v1 new API) #194

Closed
wants to merge 30 commits into from

Conversation

banshiAnton
Copy link

@banshiAnton banshiAnton commented Sep 18, 2023

Cause: GCM method which use old firebase api (will be deprecated )

  • Add new FCM method which use firebase-admin
  • Add rawPayload option for Apns method
  • Add flexible fcm_notification object param for FCM/GCM android notification send
  • android TTL calculated from expiry must not be negative

Usage:

const PushNotifications = require('./lib')

const settings = {
  fcm: {
    appName: 'MyApp',
    serviceAccountKey: require('../firebase-project-service-account-key.json')
  },
  useFCMMethodInsteadOfGCM: true
}

const pushNotifications = new PushNotifications(settings)

const tokens = ['e..Gwso:APA91.......7r910HljzGUVS_f...kbyIFk2sK6......D2s6XZWn2E21x']

const notifications = {
  collapseKey: Math.random().toString().replace('0.', ''),
  priority: 'high',
  sound: 'default',
  title: 'Title 1',
  body: 'Body 2',
  // titleLocKey: 'GREETING',
  // titleLocArgs: ['Smith', 'M'],
  // fcm_notification: {
  //   title: 'Title 1',
  //   body: 'Body 2',
  //   sound: 'default',
  //   default_vibrate_timings: true,
  // },
  // alert: {
  //   title: 'Title 2',
  //   body: 'Body 2'
  // },
  custom: {
    frined_id: 54657,
    list_id: 'N7jSif1INyZkA7r910HljzGUVS'
  }
}

pushNotifications.send(tokens, notifications, (error, result) => {
  if (error) {
    console.log('[error]', error)
    throw error
  } else {
    console.log('[result]', result, result.at(0))
  }
})

Fcm object:

{
     "data": {
          "frined_id": "54657",
          "list_id": "N7jSif1INyZkA7r910HljzGUVS"
     },
     "android": {
          "collapse_key": "5658586678087056",
          "priority": "high",
          "notification": {
               "title": "Title 1",
               "body": "Body 2",
               "sound": "default"
          },
          "ttl": 2419200000
     },
     "apns": {
          "headers": {
               "apns-expiration": "1697456586",
               "apns-collapse-id": "5658586678087056"
          },
          "payload": {
               "aps": {
                    "sound": "default",
                    "alert": {
                         "title": "Title 1",
                         "body": "Body 2"
                    }
               }
          }
     },
     "tokens": [
          "e..Gwso:APA91.......7r910HljzGUVS_f...kbyIFk2sK6......D2s6XZWn2E21x"
     ]
}

@infuzz
Copy link

infuzz commented Jan 2, 2024

HI !
Any plan to merge this great PR in the base repo ?

@miqmago
Copy link
Collaborator

miqmago commented Jan 3, 2024

@banshiAnton first of all sorry for the delay on the review and many thanks for your contribution. I think it can help to evolve the repo to better integration with FCM/GCM.

I've made some comments on your code that would help to make this repo better mantainable.

If possible, could you please add the corresponding documentation for the new options to the Readme.md, and perhaps include the examples from this pull request? It would be really helpful!

@banshiAnton
Copy link
Author

Hi @miqmago i have updated README.md
but i can't see any comments for code

I've made some comments on your code that would help to make this repo better mantainable.

@miqmago miqmago requested a review from alex-friedl January 10, 2024 17:31
@miqmago
Copy link
Collaborator

miqmago commented Jan 10, 2024

Hi @banshiAnton it's quite strange you can't see my comments... Don't appear to you on this thread? I can se them just few messages before this one, as a review... Also if you open https://github.com/appfeel/node-pushnotifications/pull/194/files?diff=split&w=1 I can see my comments there.

I've added a ping on one of them to see if you receive a message on the first one. Just let me know if it works, I will ping on every comment then.

src/push-notifications.js Outdated Show resolved Hide resolved
src/push-notifications.js Show resolved Hide resolved
src/push-notifications.js Show resolved Hide resolved
src/sendGCM.js Outdated Show resolved Hide resolved
src/utils/tools.js Show resolved Hide resolved
)(data);

const toJSONorUndefined = R.when(
R.is.bind(null, String),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here again, I'm sorry for my poor knowledge on ramda, don't understand this logic, please could you confirm that it is ok?

@alex-friedl please could you take a look too?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any scenario where using .bind() on a ramda function makes sense to be honest?
@banshiAnton Could you explain what you are trying to do here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm...
i can use only R.is(String)

const toJSONorUndefined = R.when(
  R.is(String),
  R.tryCatch(JSON.parse, R.always(undefined))
);

did not know that i can pass only first argument and ramda will return function with already binded argument
thats why i used .bind() - to bind first argument

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all ramda functions are curried by default :)
But regarding the function itself, why the check for the string? Are there are any cases where you want to return non-String inputs unchanged?

Copy link
Author

@banshiAnton banshiAnton Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for titleLocArgs and locArgs iOS alert
args should be json stringify array

{
   ...
   titleLocArgs: '["Smith", "M"]'
}

i think this is strange why i can't pass just like simple array ['Smith', 'M'] w/o JSON.stringify
so for backward compatibility i try to JSON.parse only for String type

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It's been a while but if I read through #150 I think you could simply stick with the previous implementation and only call alertLocArgsToJSON() for APN messages? Or what's the reason behind also calling toJsonOrUndefined when constructing the GSM message?

src/utils/tools.js Outdated Show resolved Hide resolved
@miqmago
Copy link
Collaborator

miqmago commented Jan 10, 2024

Ok @banshiAnton I've just noticed that I had to submit my review... 🤦🏼‍♂️🤦🏼‍♂️
You should see it now.

.eslintrc Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
)(data);

const toJSONorUndefined = R.when(
R.is.bind(null, String),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any scenario where using .bind() on a ramda function makes sense to be honest?
@banshiAnton Could you explain what you are trying to do here?

src/utils/tools.js Show resolved Hide resolved
// body: 'Body 2'
// },
custom: {
frined_id: 54657,
Copy link

@vcarballo vcarballo Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be friend_id?

@infuzz
Copy link

infuzz commented Mar 15, 2024

Hello, I just want to thank you for all your great work, as the deprecation of the old http fcm is fast approaching. I hope you have enough time to finish the new major version. Thanks again.

@miqmago
Copy link
Collaborator

miqmago commented Mar 24, 2024

From my side seems all fine.
@alex-friedl could you please confirm if is it ok for you?
@banshiAnton could you please solve package conflicts?

@alex-friedl
Copy link
Collaborator

@miqmago @banshiAnton Generally looks good to me as well.
@banshiAnton When you get to rebasing the branch, could you also take a look at the open conversations in the PR? :)

@Hossman333
Copy link

Thanks everyone for your great work here! This will be good to have so that we aren't left in a broken state in June.

@nsakaimbo
Copy link

@banshiAnton Nice work! I believe you're missing an export reference in the file lib/push_notifications.js. It currently looks like:

module.exports = PN;
module.exports.WEB = _constants.WEB_METHOD;
module.exports.WNS = _constants.WNS_METHOD;
module.exports.ADM = _constants.ADM_METHOD;
module.exports.GCM = _constants.GCM_METHOD;
module.exports.APN = _constants.APN_METHOD;

You will need to add module.exports.FCM there as well.

@miqmago
Copy link
Collaborator

miqmago commented Apr 27, 2024

@banshiAnton are you available to make pull from master, resolve conflicts and push again to your branch? This would help with merging this important feature.

@nsakaimbo
Copy link

nsakaimbo commented May 1, 2024

@miqmago 👋🏽 I left a couple of comments with some important bug fixes for this PR. My review comment is still marked as "pending" so unsure if @banshiAnton is able to see it. Specifically:

  • module.exports.FCM = _constants.FCM_METHOD needs to be added to the bottom of lib/push_notifications.js, and
  • The TTL reference in src/utils/fcmMessage.js needs to be fixed so it reads androidMessage.ttl = androidMessage.timeToLive (i.e. timeToLive should not be snake-cased here). In addition, the * 1000 multiplier could cause issues as it looks like this value is expected to be in seconds, not ms per the FCM docs

@ehaynes99
Copy link

In the linked issue in node-gcm, the OP assumes that since it was deprecated on June 20, 2023, it will be discontinued on June 20, 2024, but Google doesn't actually give a day, only a month. It could as easily be June 1, so we're 3 weeks away.

Since we haven't heard from @banshiAnton since January, I forked the fork, updated it with current master, and addressed the feedback by @nsakaimbo

I have opened a PR here: #205

Just to be clear, I'm not trying to steal credit in any way, and I don't pretend to have much domain knowledge nor a full grasp of all of the preceding changes. But my company depends on this library and will be in serious trouble without the update.

@pushchris
Copy link
Contributor

Deadline is fast approaching on this. When is this going to be merged and is there anything the rest of us can do to help expedite?

@Arpanexe
Copy link

Any plan to merge this PR?

@NicolasBonduel
Copy link

NicolasBonduel commented Jun 3, 2024

It looks like Google has updated their documentation, and now says the API will stop working on the 21st of June.

@ehaynes99
Copy link

FWIW, here's a general example of what I ended up with using firebase-admin directly. I closed my copy of this PR because it wasn't working correctly, and it was too painful trying to reverse engineer all of the transformations in here to map to the firebase syntax.

import type firebase from 'firebase-admin'
import type { FirebaseError } from 'firebase-admin'
import type { TokenMessage } from 'firebase-admin/lib/messaging/messaging-api'

// 4 weeks - the maximum
const DEFAULT_TTL = 4 * 7 * 25 * 60

export type MyCustomPushNotification = {
  title: string
  body: string
  collapseKey?: string
  priority?: 'high' | 'normal'
  // any other properties
  data: Record<string, unknown>
}

export type FcmClient = {
  sendAndroid: (token: string, notification: MyCustomPushNotification) => Promise<void>
}

export const FcmClient = (firebaseApp: firebase.app.App): FcmClient => {
  return {
    sendAndroid: async (token, notification) => {
      const { collapseKey, priority = 'high' } = notification

      // Types: https://github.com/firebase/firebase-admin-node/blob/4070f5bf41f83368ec8f7c4f3136a25dfcdf3e03/src/messaging/messaging-api.ts#L20-L27
      const message: TokenMessage = {
        token,
        // see https://firebase.google.com/docs/cloud-messaging/concept-options#notifications_and_data_messages
        // in short, you can have either or both of:
        // `data` can only be handled by the client app
        data: serializeData({
          title: notification.title,
          body: notification.body,
          ...notification.data,
          // any other properties
        }),
        // `notification` will be displayed by the phone OS unless the app is in focus
        notification: {
          title: notification.title,
          body: notification.body,
        },
        // you can send to multiple platforms. see: https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages
        // I only used android here like so:
        android: {
          collapseKey,
          priority,
          ttl: DEFAULT_TTL,
        },
        // the formats for other types are different, see the link above
        // apns: {
        //
        // },
        // webpush: {
        //
        // },
      }
      try {
        await firebaseApp.messaging().send(message)
      } catch (error) {
        console.error('FCM error:', { error, message })
        if (isFirebaseAuthErrorError(error)) {
          // invalidate the token in your app
        } else {
          throw error
        }
      }
    },
  }
}

const isFirebaseAuthErrorError = (error: unknown): error is FirebaseError => {
  return error instanceof Error && 'code' in error && error.code === 'messaging/registration-token-not-registered'
}

/**
 * `data` type for FCM must be a `Record<string, string>`. You can handle this however you like, but the way
 * we're doing it here is to to serialize any values that are not strings as JSON. The client will need
 * to parse these values back into objects.
 * Values that are already strings are left as is to avoid double encoding.
 */
const serializeData = (data: Record<string, unknown>): Record<string, string> => {
  return Object.fromEntries(
    Object.entries(data).map(([key, value]) => [key, typeof value === 'string' ? value : JSON.stringify(value)]),
  )
}

@wcalebgray
Copy link
Contributor

I copied over the code from this PR to my fork and got all of the test cases passing. I'll be trying to get this to work tomorrow, but any help would be appreciated. Maybe we can still get this merged and updated before Friday.

@alex-friedl
Copy link
Collaborator

Superseded by #207

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.