parse-community / parse-server-push-adapter

A push notification adapter for Parse Server
https://parseplatform.org
MIT License
85 stars 100 forks source link

feat: add support for APNS `interruption-level` and `target-content-id` #197

Closed mman closed 2 years ago

mman commented 2 years ago

This PR along with https://github.com/parse-community/node-apn/pull/87 adds support for specification of target-content-id and interruption-level in APNS push notification payload to address https://github.com/parse-community/parse-server-push-adapter/issues/196 and https://github.com/parse-community/parse-server-push-adapter/issues/155.

mtrezza commented 2 years ago

@mman Do you want to fix the failing tests to get this ready for review? I'll update to node-apns 5 today as well for a new release.

mman commented 2 years ago

@mtrezza the tests should pass once the updated node-apn is installed. I will take a look tomorrow European time…

mman commented 2 years ago

@mtrezza Looks like the node apn version 5 was released but does not contain the necessary changes that are in node apn master. 5.0.1 will be needed I guess https://github.com/parse-community/node-apn/compare/5.0.0...master

mtrezza commented 2 years ago

@mman released 5.1.0

mman commented 2 years ago

@mtrezza JavaScript and npm are not my best friends, but I believe that npm was sticking to 5.0.x updates only and that is why it would not pick up 5.1. So I updated package.json to depend on 5.1.x and tests seem to work as expected now.

mtrezza commented 2 years ago

I updated package.json to depend on 5.1.x

Did you do that with npm i -E ...@5.1.0 or only edit the package.json file?

mman commented 2 years ago

I updated package.json to depend on 5.1.x

Did you do that with npm i -E ...@5.1.0 or only edit the package.json file?

I used vim(1) :-)

/me going to check what npm i -E does

mtrezza commented 2 years ago

Sorry for the confusion; I mean to ask whether you installed the package using npm i -E ... or whether you manually edited package.json, deleted package-lock.json and called npm i to re-create package-lock.json. Because the first one would be the way to go.

mman commented 2 years ago

Sorry for the confusion; I mean to ask whether you installed the package using npm i -E ... or whether you manually edited package.json, deleted package-lock.json and called npm i to re-create package-lock.json. Because the first one would be the way to go.

I edited the package.json, updated the required version of node-apn to 5.1.0 and ran npm install to generate package-lock.json and then committed both package.json and package-lock.json to the tree.

I am not a JavaScript/npm guru so feel free to do whatever is needed to cleanup the PR in a way suitable for merge. The key is that in order for tests to pass, node-apn needs to be at least 5.1.0.

mtrezza commented 2 years ago

I edited the package.json, updated the required version of node-apn to 5.1.0 and ran npm install to generate package-lock.json and then committed both package.json and package-lock.json to the tree.

That's probably why this PR contains such 6k lines of change. What you want to do is:

  1. Revert package.json and package-lock.json as they were before this PR
  2. run npm i -E parse-community/node-apn@5.1.0, this will update only the existing node-apn dependency with the never version, without touching any other dependencies
  3. commit package.json and package-lock.json

Regenerating the whole package-lock.json file means that other sub-dependencies would be changing as well, which we do not want, unless it is a PR dedicated to regenerating the package-lock.json file. See https://github.com/parse-community/parse-server/issues/7417.

mman commented 2 years ago

@mtrezza I did precisely that and the results appear to be the same, the package-lock.json contains everything updated to the latest versions. Here is my setup:

[~/Developer/parse-server-push-adapter] (master) € node --version
v16.13.0
[~/Developer/parse-server-push-adapter] (master) € npm --version
8.1.0
mtrezza commented 2 years ago

I notice you are also on npm 8, that's another reason why there are so many changes. Can you downgrade to node 12 to upgrade the dependency? We always make changes in the lowest supported npm version to ensure backward compatibility.

mman commented 2 years ago

That seems to have done the trick. Thanks for your help, although I will not claim I understand why npm behaves this way.

[~/Developer/parse-server-push-adapter] (master) € git reset --hard c57e5bab3c9736f662dd7053bcbd43c105e44bea
HEAD is now at c57e5ba Merge branch 'parse-community:master' into master
[~/Developer/parse-server-push-adapter] (master) € export PATH="/usr/local/opt/node@12/bin:$PATH
dquote> 
[~/Developer/parse-server-push-adapter] (master) € export PATH="/usr/local/opt/node@12/bin:$PATH"
[~/Developer/parse-server-push-adapter] (master) € node --version
v12.22.7
[~/Developer/parse-server-push-adapter] (master) € npm --version
6.14.15
[~/Developer/parse-server-push-adapter] (master) € npm i -E @parse/node-apn@5.1.0
mtrezza commented 2 years ago

The reason is that npm >=7 uses another dependency management tree; you can see that the lock file has version 2 instead of version 1. That causes many changes. If you want to find out more you can search online for the difference between lock file versions 1 and 2.

mtrezza commented 2 years ago

@mman Is this ready for review?

mman commented 2 years ago

@mman Is this ready for review?

Yes please

parseplatformorg commented 2 years ago

🎉 This change has been released in version 4.1.0

parseplatformorg commented 2 years ago

🎉 This change has been released in version 4.1.0