Support for LTS 13.0, remove Jose, polymorphic messages

Znack commented 5 years ago

Hello @sarthakbagaria!

Thanks for the library, we are gonna use it in our project and would like to help you in maintaining :)

We have added the support for LTS 13.x series, move from Jose, make the payload polymorphic and other minor fixes.

We would like to hear your feedback is our way appropriate.

sarthakbagaria commented 5 years ago

Hi @Znack, @NikitaRzm,

Thank you very much for your work on this library and the offer to help maintain. I reviewed the changes and have a few concerns:

Any reason why you want to remove use of Jose? It seems to be included in LTS 13.0.
I am in full support of making the payload polymorphic. But can we also keep PushNotificationMessage around so that users of the library have a datatype to refer to when creating a standard push message for web?

A couple of other minor points just wanted to mention. Please let me know your thoughts.

I generally prefer qualified imports or explicitly importing functions/operators from libraries, rather than importing whole libraries. Makes it easier to look up which library a function was taken from.
We should update the version of the library if we are changing the exposed API (PushNotification).
We should probably not leave print statements in the library.

NikitaRzm commented 5 years ago

Hi, @sarthakbagaria ! Thanks for your feedback! I will refactor code according your comments:

fix imports;
remove print expressions;
and move back standard payload as quick to use data structure.

About JOSE: in 0.8.0 version constructors of MaterialKey datatype became closed, so i can't make JWT from this. Now MaterialKey with given Curve can be created through smart-constructor, but it also will generate other components too, so we can't inject our parts of VAPID keys. I tried to inject payload by JSON (non-typesafe) (probably it needs to be squashed, cause commits history is pretty dirty) parsing in Material, but web-pushes did not worked with its implementation.

I tried your manual algorithm and it works perfectly! But i also ready to try any other ways, that you will offer, cause now i have no good ideas for that :(

Also, i want to note, that manual implementation without any libs is very transparent for users and completely corresponds to documentation about encrypting pushes :)

sarthakbagaria commented 5 years ago

@NikitaRzm Makes perfect sense. I had written the manual implementation before I realised I could use Jose to do that same thing, good thing I did that it seems. :)

Will merge as soon as you have committed the suggested changes. Also can you please update the version of the library to 0.2 from 0.1.2.0 in the cabal file along with these changes as we are upgrading the API. Thank you!

NikitaRzm commented 5 years ago

@sarthakbagaria , i have fixed code issues, that you mentioned. Please, check if it has no other issues now and that we are ready to new version, thank you! :)

Znack commented 5 years ago

@sarthakbagaria thanks for merging, it is awesome! :)

sarthakbagaria commented 5 years ago

@Znack @NikitaRzm Thank you for your help on this library! :)

sarthakbagaria / web-push

Support for LTS 13.0, remove Jose, polymorphic messages #1