Amazon SES Engine support and example #38

Merged
merged 5 commits into from Aug 12, 2011

Projects

None yet

2 participants

@dfellis
dfellis commented Aug 12, 2011

Hi Andris,

This took longer than I thought (Amazon's documentation was difficult to follow), but I finally got Amazon SES support working without resorting to adding a dependency on aws-lib.

Please let me know what you think of the code, and if there are any changes you'd like me to make.

Regards,
David

@andris9
Member
andris9 commented Aug 12, 2011

Thanks! I'll try to get a SES account for myself and then test this out.

From a quick peek the structure of the code seemed OK. I'll just point out that it is not the best idea to use for .. in construct for iterating object properties - usually it is better to use Object.keys instead

var keys = Object.keys(obj);
for(var i=0, len = keys.length; i<len; i++){
    console.log(obj[keys[i]]);
}

Probably this won't ever be a problem but as this is a module to be used by unknown projects then you'll never know what might happen.

@andris9 andris9 merged commit 36f6831 into nodemailer:master Aug 12, 2011
@dfellis
dfellis commented Aug 12, 2011

Yes, I suppose someone may generate the SES object with a prototype and cause extra attributes to be attached that I'm not expecting. I hadn't though of any case other than JSON, since there are only two required parameters. (The other way would be more difficult to produce.)

I'm only using for .. in in one place; do you want to make the change, then?

Oh, and when can we expect a new version tag with accessibility through NPM? :)

@andris9
Member
andris9 commented Aug 13, 2011

I'll try to review and test everything over the weekend and then release the npm package. If you need to use the new version already you could use npm ln to link the module to a local directory.

@andris9
Member
andris9 commented Aug 13, 2011

Btw. It's not a SES object that might be "compromised" but any objects in general, consider the following scenario

Object.prototype.abc = 1;
for(var i in {}){alert(i)} // "abc"
@andris9
Member
andris9 commented Aug 13, 2011

Hi,

I edited the code a bit (some cosmetic changes like replacing tabs with spaces etc.) and published it as v0.2.0 to npm, also tagged it in git.

Thanks for the great work!

@dfellis
dfellis commented Aug 14, 2011

I see about the object issue and how it would need to be guarded against. Isn't that on the list of dumbest things you could do in Javascript, though?

As far as your changes cosmetically, I'm fine with that. I'm just a tab guy myself because:

  1. I've worked with programmers who would accidentally delete spaces and commit it as is, leaving jagged lines that aren't immediately noticeable but then make visual inspection of blocks difficult.
  2. You can configure your editor to represent tabs in as many spaces as you want, so no 2-space, 4-space, 8-space fighting between devs on a team.
  3. Multi-line comments to the right of code are a dumb idea and don't need to be catered to at all, in my opinion.

And thank you for this very useful library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment