This module shouldn't enforce the use of dotenv

samholmes commented 8 years ago

It doesn't make sense to me why this module would be concerning itself with a security decision such as using a .env file for environmental configuration. It should be left up to the developer to choose how they want to handle configuration for their app per environment.

Also, it seems weird to me that the module wouldn't explicitly ask for the API key, but rather assumes it should be stored in a .env file. What if the app isn't using a .env file for the app's secret configuration information? Now the developer is forced to have a .env file along with their existing environmental configuration implementations. Or, if they choose not to have a .env file, the module will complain about it in the logs. This doesn't make sense to me.

What would make sense to me is if they module just accepted and required a SparkPost API key in the options and left the configuration up to the developer. The module shouldn't have an opinion on how this information is passed to the module.

However it could be suggested by SparkPost to use dotenv module, but that should still only be a suggestion. And, the implementation of a dotenv configuration into an app should be transparent, not hidden within a module's implementation (If I search for the a configuration within my app and don't find it unless I include dependencies in that search, then that's bad for debugging or readability).

I hope I made my case. I suggest that you require the "sparkPostApiKey" option to be set when a transport is created and leave it up to the developer to choose to use dotenv or some other solution for how that key is set within their app.

crobinson42 commented 8 years ago

@samholmes I agree with your remarks. I switched to using the official Sparkpost npm module because it's much more simple and maintained up to date with the available Sparkpost API.

ie:

var SparkPost = require('sparkpost');
var SparkPostClient = new SparkPost('xxxxxxxxx-xxxxxx-xxxxxxxxx');

var reqObj = {
          transmissionBody: {
            content: {
              from: 'me@example.com',
              subject: 'subject',
              html: '<html><body>...',
              text: 'text body....'
            },
            recipients: [ 'you@example.com' ]
          }
        };

SparkPostClient.transmissions.send(reqObj, function(err, res) { ... });

richleland commented 8 years ago

I completely agree - thanks for opening this ticket @samholmes. We can target getting this updated for the next release. And to your point @crobinson42 we really do need to be keeping this updated and in sync with our node-sparkpost library. Thanks for the nudge you two!

samholmes commented 8 years ago

I switched to the official sparkpost module instead of using the nodemailer transport. Although, thanks for bring dotenv to my awareness as I may utilize that in my app! :)

djensen47 commented 8 years ago

:+1: I'm switching from Mandrill to Sparkpost. I'm already using nodemailer so I was hoping this would be a simple drop-in replacement but I'm using node-config for my config and not environment variables.

SwiftRabbit commented 8 years ago

I have no choice to do as @samholmes did, considering the .env is still required.

aydrian commented 8 years ago

I should be able to pull that out. I prefer to use autoenv which just loads variables from a .env file in a directory. I'll try to get a release out tomorrow.

djensen47 commented 8 years ago

Sorry, I got things a little mixed up in my first few drafts of this comment.

For me the issue is/was:

Reducing dependencies
Following the pattern used by other nodemailer transports, which, it turns out, this library does indeed do
Documenting that sparkPostApiKey is required in order for this transport module to work

I take it autoenv is something else entirely and not a node module?

aydrian commented 8 years ago

I can definitely update the docs. autoenv is just a handy little tool that will load values from a .env file in a directory. It's very helpful. For all my projects, I have a .env file that holds the environment variables needed for that project. Whenever I cd into the directory, it loads them. We should not be forcing variables to load a certain way in this library. If you use Heroku, they have a setup for loading variables as does most hosting environments. It looks like we mainly need to better document how you can pass in a key without using an env var. I'm also going to remove the dependency for dotenv because it shouldn't be needed.

aydrian commented 8 years ago

This has been released to NPM as v0.1.2.

SparkPost / nodemailer-sparkpost-transport

This module shouldn't enforce the use of dotenv #3