Code review

[josephg]

As requested, here's a code review. I couldn't figure out how to review an entire repo using github's built in code review tooling, so my comments are below:

General:

• DRY. Dry dry dry dry dry.
• Nowhere near enough comments. In Email.js There are 3 lines of comments in 87 lines of code. All public interface methods should have a comment explaining what they do and what arguments they expect.
• You're handling lots of input different data types in some quite deep functions. You're doing this without any documentation about what types of inputs the functions expect (Eg, mailgun/getRecipientsAndVars.js supports to as a list or a single item, and it can be a list of strings or a list of objects). This is a bad idea: With no documentation, anyone trying to maintain this code has no idea what inputs the functions are supposed to support. Also, all the type checks obscure what your functions are actually trying to do. Consider normalizing your input on the way in then simplifying the code in your functions. (This is also good for the optimizer).

  Email.js

https://github.com/keystonejs/keystone-email/blob/master/lib/Email.js#L10

Constructor takes options object. What properties are avaliable on options? Consider adding a documentation comment to the function listing what options it supports.

https://github.com/keystonejs/keystone-email/blob/master/lib/Email.js#L35-L36

    this.root = options.root || process.cwd();
    this.template = this.resolve(template);

Depending on cwd is bad practice - it makes applications brittle to how they're invoked. Consider logic closer to express's view engine: https://github.com/expressjs/express/blob/master/lib/view.js#L95-L114

https://github.com/keystonejs/keystone-email/blob/master/lib/Email.js#L48-L54

Thats a lot of code to make the callback and options both optional. Consider:

    callback = callback || function () {};
    this.engine(this.path, options || {}, function (err, html) {
        ...

https://github.com/keystonejs/keystone-email/blob/master/lib/Email.js#L65-L71

I had to stare at this code for about 5 minutes to figure out what its trying to do. I'm pretty sure its buggy but I can't tell for sure because I can't tell what its expected behaviour is supposed to be.

> name = 'tarsnap.key'
> var loc = path.resolve('/Users/josephg', name);
'/Users/josephg/tarsnap.key'
> var dir = path.dirname(loc);
'/Users/josephg'
> var file = path.basename(loc, '.key');
'tarsnap'
> var filepath = path.join(dir, file);
'/Users/josephg/tarsnap'
> isFile(filepath) // false, because you stripped off the file's extension.

Is this behaviour correct?

https://github.com/keystonejs/keystone-email/blob/master/lib/Email.js#L73

What values can I pass to renderOptions and sendOptions? Comment please.

https://github.com/keystonejs/keystone-email/blob/master/lib/Email.js#L83-L85

If this is the main entrypoint, isFile should be async and use stat instead of statSync.

util/Truthy.js

This function is unnecessary, and should be inlined where its used. Every javascript programmer should know what !!val means. (And if they don't, they won't be able to read your truthy function either). Don't make me hunt down obscure files unnecessarily.

util/cleanHTML.js

What does this file do? Are there security implications here? Is unicode still supported? A lot of thought has obviously gone into this code. It is impossible to maintain this function (or even tell whether this function is correct) without that context, which is not obvious and should exist next to the code itself.

util/getTransport.js

Should this support third-party transports the same way as the getEngines.js file does?

• If yes, the code is incorrect.
• If no, consider moving this logic into lib/transports/index.js.

  util/isFile.js

As far as I can tell this function is invoked on every email render call. Either cache the result or use fs.stat instead of fs.statSync.

---

Transports

General: These probably want to be their own modules, though I can see why you're inlining them for now.

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mailgun/index.js#L22-L23

Its sort of weird copying the options into a new object, then deleting some of them again. I feel like this code should either use single-static-assignment for options, or treat options as mutable. Mixing both of those styles feels a little icky.

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mailgun/processAddress.js

General: Are < and > valid characters in email addresses?

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mailgun/processAddress.js#L3

Should be /^.<(.)>$/; or /<(.*)>$/;

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mailgun/getRecipientsAndVars.js#L8-L9

Don't call the variable holding the address i. Save i for the index.

On that topic, what is the variable 'vars' supposed to represent?

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mailgun/getRecipientsAndVars.js#L19

    if (!vars[rcpt.email].email) vars[rcpt.email].email = rcpt.email;

Huh? So, vars['dave@example.com'].email = 'dave@example.com' ? Why?

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mandrill/getRecipientsAndVars.js

DRY. This function is an obvious copy+paste job of mailgun/getRecipientsAndVars.js. Extract out the common functionality for code sharing, and the different functionality to make the differences obvious.

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mandrill/objToMandrillVars.js

Add a comment:

// A mandrill vars object is nested like this {... example here}. We need to flatten it out for reasons A and B

https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mandrill/index.js

This file looks like another copy+paste job from mailgun/index.js, with more arbitrary changes. DRY.

[JedWatson]

Thanks @josephg! Awesome review.

Updates for those playing along at home:

• [x] DRY. Dry dry dry dry dry.
  • Resolved where possible
• [ ] Nowhere near enough comments.
• [ ] You're handling lots of input different data types in some quite deep functions.
• [x] What properties are avaliable on options? Consider adding a documentation comment to the function listing what options it supports
  • I'm just going to reference the readme to limit double-handling, it's all documented in there and I prefer a single source of truth
• [x] Depending on cwd is bad practice
  • Actually this entire logic block is based on the code you linked to in express 😜
• [x] Bug in Email.prototype.resolve - resolved, good catch! Have added comments.

Still need to go through the notes for other files and transports, will add another comment when I do.