-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
b7bdb60
to
e751603
Compare
Would it be worth changing this to just be steps to use rather than an explainer of the API? I'd expect API docs to explain the API itself and the README be the rough steps to get this working.
Question regarding API - was there a reason for not handling the authentication header inside the library rather than the developer having to manage it? |
Does that make sense? The actual |
|
||
Require the module: | ||
|
||
`const webpush = require('web-push-encryption');` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I thought the plan for ES2015 usage was to use it in the libraries, but keep samples and docs in plain JS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never heard this being discussed but it's a good point - it probably should be.
Added a few comments, if you gave this to me today and said go, it would take me too long to figure out how to use it. I want a quick start explanation to get me going immediately, then go from there. |
Ta for the explanation, still not entirely convinced by this API though. 1.) If it's only used by GCM, then should it just be of addGCMAuthToken(''); webpush.addGCMAuthToken(''); Essentially - I just really dislike the pattern requirement - feels to error prone and boilerplate. SIDE NOTE: This should probably be moved to an issue and doesn't block these this docs PR. |
+1 Peters comments. I think keep this on the bare minimum titled getting started to just install and use the library for Chrome and Firefox. We can either than add further comments in API docs OR seperate sections. For now I'd go minimal and start asking people for feedback and iterate on it. |
Moved auth token comments to: #6 would appreciate any feedback. |
@gauntface @petele I simplified the docs, removing the API reference. I also simplified the auth token business - auth token is now a parameter of sendWebPush and we push the process onto the developer. This came up in the code review for the Go version, too, and in the end it seemed like it was actually easier for developers to grok what was going on if they were left in control. |
|
||
Install the module using npm: | ||
|
||
`npm install web-push-encryption` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the name we are actually published on? Does it make sense to make it slightly more generic if we can support encrypted push and just tickles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted web-push
but Mozilla got there first :)
Will open an issue.
LGTM Commented on the naming of the module, but that can be moved to an issue. |
R: @kaycebasques @gauntface
CC: @petele