Skip to content

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

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

Merged
merged 3 commits into from
Nov 21, 2021

Conversation

mman
Copy link
Contributor

@mman mman commented Nov 12, 2021

This PR along with parse-community/node-apn#87 adds support for specification of target-content-id and interruption-level in APNS push notification payload to address #196 and #155.

@mtrezza
Copy link
Member

mtrezza commented Nov 17, 2021

@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
Copy link
Contributor Author

mman commented Nov 17, 2021

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

@mman
Copy link
Contributor Author

mman commented Nov 17, 2021

@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 parse-community/node-apn@5.0.0...master

@mtrezza
Copy link
Member

mtrezza commented Nov 17, 2021

@mman released 5.1.0

@mman
Copy link
Contributor Author

mman commented Nov 18, 2021

@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
Copy link
Member

mtrezza commented Nov 18, 2021

I updated package.json to depend on 5.1.x

Did you do that with npm i -E [email protected] or only edit the package.json file?

@mman
Copy link
Contributor Author

mman commented Nov 18, 2021

I updated package.json to depend on 5.1.x

Did you do that with npm i -E [email protected] or only edit the package.json file?

I used vim(1) :-)

/me going to check what npm i -E does

@mtrezza
Copy link
Member

mtrezza commented Nov 18, 2021

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
Copy link
Contributor Author

mman commented Nov 19, 2021

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
Copy link
Member

mtrezza commented Nov 19, 2021

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/[email protected], 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 parse-community/parse-server#7417.

@mman
Copy link
Contributor Author

mman commented Nov 19, 2021

@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
Copy link
Member

mtrezza commented Nov 19, 2021

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
Copy link
Contributor Author

mman commented Nov 19, 2021

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/[email protected]

@mtrezza
Copy link
Member

mtrezza commented Nov 19, 2021

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
Copy link
Member

mtrezza commented Nov 21, 2021

@mman Is this ready for review?

@mman
Copy link
Contributor Author

mman commented Nov 21, 2021

@mman Is this ready for review?

Yes please

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good!

@mtrezza mtrezza changed the title add support for "interruption-level" and "target-content-id" feat: add support for APNS interruption-level and target-content-id Nov 21, 2021
@mtrezza mtrezza merged commit e0541ec into parse-community:master Nov 21, 2021
parseplatformorg pushed a commit that referenced this pull request Nov 21, 2021
# [4.1.0](4.0.0...4.1.0) (2021-11-21)

### Features

* add support for APNS `interruption-level` and `target-content-id` ([#197](#197)) ([e0541ec](e0541ec))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 4.1.0

parseplatformorg pushed a commit that referenced this pull request Nov 21, 2021
# [4.1.0](4.0.0...4.1.0) (2021-11-21)

### Features

* add support for APNS `interruption-level` and `target-content-id` ([#197](#197)) ([e0541ec](e0541ec))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 4.1.0

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

Successfully merging this pull request may close these issues.

3 participants