Support apns http2 node apn version#134
Conversation
superkhau
force-pushed
the
support-apns-http2-node-apn-version
branch
from
467c68b to
d668f92
Compare
|
@raymondfeng The tests are failing because the new node-apns uses es6 enhanced object literal syntax: https://github.com/node-apn/node-apn/blob/master/index.js#L28. Merging this will mean we're not supporting node 10/12 anymore. Will ping you in the morning to confirm how to move forward. |
|
Hmm, that's annoying. I guess we can potentially release a new major version that requires Node 4 or later. |
|
I agree with your proposed solution because I don't see simpler way around this without forking and maintaining 2 versions. We're dropping 10 soon anyways and 12 in like another month or so: strongloop/loopback#2807. The question is whether you want to just land this now or wait for LB to officially drop support. @bajtos Thoughts? |
|
We should start a new major version of loopback-component-push first and drop support for Node v0.10/v0.12 there. This pull request should be landed on the new master and should not affect current version line 1.x. Dropping support for Node v0.10/v0.12 is basically waiting for our CI build-framework to recognize |
|
Now that #135 was landed, we can move forward with this patch too. @superkhau |
|
|
||
| it('sends Notification as an APN message', function(done) { | ||
| givenProviderWithConfig(); | ||
| givenProviderWithConfig(defaultConfiguration); |
There was a problem hiding this comment.
The idea of givenProviderWithConfig is to create a provider with sensible default configuration, so that we don't have to repeat ourselves in the tests and also don't have to change all tests when the default configuration changes like here.
Please change givenProviderWithConfig to set sensible apns defaults and revert changes in this file.
| done(); | ||
| // HACK: Timeout does not work at this point | ||
| Promise.resolve(true).then(function() { | ||
| assert(eventSpy.called); |
There was a problem hiding this comment.
You lost the assertion on notifications reported:
expect(eventSpy.args[0]).to.deep.equal([devices]);| interval: 300, | ||
| }, | ||
| // Sign up an application | ||
| Application.register('test-user', 'TestApp', { |
There was a problem hiding this comment.
This looks like an unrelated whitespace change to me, can we please revert?
| pushNotification: function(notification, deviceToken) { | ||
| // console.log(notification, deviceToken); | ||
| assert.equal(deviceToken, result.deviceToken); | ||
| assert.equal(deviceToken, deviceToken); |
There was a problem hiding this comment.
Asserting that X == X makes no sense as it always passes. Please revert this line.
|
@superkhau I will address those issues and deliver a solution within this week if possible. |
|
@iliraga No problem, just ping again when you need review/help landing after your updates. Let's continue the conversation in your other PR. I'm going to close this and try to land your original PR since you'll be pushing updates anyways. Please rebase it with master while you're at it. |
4 participants
Helping land #133
cc @iliraga @raymondfeng