PeterStaev / nativescript-azure-mobile-apps

:cloud: NativeScript plugin for working with Microsoft Azure Mobile Apps services
Apache License 2.0
30 stars 10 forks source link

Push notifications (implements #7) #9

Closed georgeedwards closed 8 years ago

georgeedwards commented 8 years ago

Hi Peter,

Just as a follow up from the Issue I opened, I do believe that Push Notifications (with Azure Notification Hubs) is indeed part of this SDK.

I am trying to do this in a style concurrent with your other work.

Is this something you faced at any other point in the development of the plugin?

Note: the first 4 commits are undoing the work we last spoke about.

PeterStaev commented 8 years ago

Hey @georgeedwards , thanks for your contribution! After further looking at the push supported by azure , seems you were right and it is part of the Mobile SDK. Sorry initially What i've seen seems it was part of a separate SDK (like the blob storage).

Now on topic - it is a bit hard to read your exact changes as you made quite a lot of changes to other files and also included the build artifacts in the report, which is really bad. So you will need to revert those changes. But from what i see you are trying to send a JS object (client) to both native methods on android and iOS. I'm not sure even how it is working on android. You will need to send the native value which is in the protected _msClient of the client object. Otherwise i suspect it will not work. Also for android all those methods that return Listenable should return a promise instead like I do for the other methods in query/table.

As a general rule please try to limit your changes to what you are implementing. Like for this change the changed files should be something like 5-8 (add 5 files for the push module, may be some changes to the rest), not 28. Otherwise it makes changes really unreadable and I can't really see what you changed.

PeterStaev commented 8 years ago

I would strongly suggest that you create a new branch from the master and then put only the push files and relative changes and submit another PR. This should clean up the past bulky commits & unreverted changes. Thanks!