-
-
Notifications
You must be signed in to change notification settings - Fork 111
feat: add http adapter #231
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
Conversation
lib/notification/adapter/http.js
Outdated
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${authToken}`, |
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 think it makes sense to make the auth optional, don't you think?
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.
yes, definitely the right call. I've made the changes
lib/notification/adapter/http.js
Outdated
| endpointUrl: { | ||
| type: 'text', | ||
| label: 'Endpoint URL', | ||
| description: "'Your application's endpoint URL.", |
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.
A typo. You have one ' too much :)
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.
whoops, fixed!
lib/notification/adapter/http.md
Outdated
| @@ -0,0 +1,31 @@ | |||
| ### HTTP Adapter | |||
|
|
|||
| This is a generic adapter for sending notifications via HTTP requests. | |||
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.
You're not mentioning the auth token here. I'd make it optional, but describe how to use it in any case. Note, not all users are pro's ;)
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've improved the docs a bit 👍
| type: 'text', | ||
| }, | ||
| authToken: { | ||
| description: "Your application's auth token, if required by your endpoint.", |
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'll merge it this time, but in the future, please make sure to install the pre-commit hooks. If you do, it would have told you that double quotes are not allowed. I'll fix this later
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.
This string has a single quote inside, therefore it doesn't raise any issues. This is how similar usages are handled (e.g. lib/notification/adapter/telegram.js:122)
Adds a generic HTTP adapter for making a
POSTrequest at a target URL after new listings are found.My use case would be to have an endpoint, that I can make a request to when new listings are found. This way the user could create long-running servers, or serverless functions to do additional operations.