-
Notifications
You must be signed in to change notification settings - Fork 23
Image attachment feature #23
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
|
Awesome work! Unfortunately I am pretty adamant about not adding dependencies. Any chance you can re-work this to not use form-data? |
Thanks @qbit .
Unfortunately I don’t have time to re-implement the form-data module. And probably wouldn’t do as good a job as those who have already solved this problem anyway. I can’t say I understand why you insist on not adding dependencies, but of course I respect your decision. There is demand for this feature, and I too need it for my own work. If you change your mind, do let me know before the weekend. Otherwise, I’ll just publish a fork under the terms of your licence (with full attribution to your work of course). Damo. |
Appreciate it! A bit of an explanation: Just in adding that one module introduces a total of 6 new modules that weren't there before. The total lines of JS added by these six modules is 1050. This library is only 245 lines of JS currently (346 with the attach implementation I just pushed in #24). Anway - let me know if the implementation I just pushed works for you. If not feel free to fork away :D! |
|
Hey Aaron, Glad you were able to find time to implement the feature. I understand coding in your personal time can be challenging.
I had surmised your reasoning was in relation to optimisation (storage and/or performance). What I don't understand is why you value it so highly, at the expense of so many other factors, such as:
Unfortunately your method of optimisation (no dependencies) comes at a cost. So my question is, as an uncompromising principle - is it worth it? I’m not at all telling you how to code. It goes without saying that this is your module and you can build it as you see fit. Quite likely, there are external factors influencing your decision-making that I am unaware of. I’m just offering an alternate perspective for your consideration. Cheers, |
|
It has more to do with a distrust of npm. Dependencies increase exposure to things like this. Regarding readability, I feel that's debatable. The entire form handler in #24 is contained in one js file.. with significantly less code (typically code is easier to understand when there is less going on.. at least in my experience). I do agree that the specific diff is cleaner. |
Agreed, there is an element of subjectivity to this. Deconstructing the problem through module abstractions helps with readability which you automatically get with installed modules. You don't need to concern yourself with the implementation. Admittedly, you could have written the form handler in a separate file to achieve the same readability gains.
Given your explanation, I hadn't considered mistrust of the npm community as your primary concern with dependencies. I understand people value trust differently and respect your position. Based on my experiences and context, my position is a little different. I'm happy to trade a little trust in exchange for the benefits of others' work. Thanks for engaging in this discussion Aaron. Regards, |
1 similar comment
Agreed, there is an element of subjectivity to this. Deconstructing the problem through module abstractions helps with readability which you automatically get with installed modules. You don't need to concern yourself with the implementation. Admittedly, you could have written the form handler in a separate file to achieve the same readability gains.
Given your explanation, I hadn't considered mistrust of the npm community as your primary concern with dependencies. I understand people value trust differently and respect your position. Based on my experiences and context, my position is a little different. I'm happy to trade a little trust in exchange for the benefits of others' work. Thanks for engaging in this discussion Aaron. Regards, |
Fair point as there is an element of subjectivity to this. Deconstructing the problem through module abstractions helps with readability which you automatically get with installed modules. You don't need to concern yourself with the implementation. Admittedly, you could have written the form handler in a separate file to achieve the same readability gains.
Given your explanation, I hadn't considered mistrust of the npm community as your primary concern with dependencies. I understand people value trust differently and respect your position. Based on my experiences and context, my position is a little different. I'm happy to trade a little trust in exchange for the benefits of others' work. Thanks for engaging in this discussion Aaron. Regards, |
Hi @qbit
This PR implements the attachment feature of the pushover API as per #20 so that you can send pushover messages with images.
The implementation uses the form-data module to generate the
multipart/form-dataencoding. The module is downloaded 10M weekly, is robust and succinct in implementation.I've prepared tests to validate the new functionality, and existing tests still pass.
I've also updated the README with examples of how to send images with messages according to this PR. Images can be provided using a ReadStream of a Buffer.
@qbit would you merge this PR into master?
Cheers,
Damo.