Goodparts refactor

[newswim]

Hello! This new PR is meant to add GoodParts compliance a la Issue #66 and re-initialize the merge from within the upstream repo rather than my own fork.

It has the following TODOs:

• sendemail is not returning anything, so tests 10 and 11 never end (see failure in action).

[samhstn]

I think the way to ensure the last 2 tests pass is to rewrite the compile_template function.

The last 2 tests currently is breaking because the filepath variable is being evaluated when the process.env.TEMPLATE_DIRECTORY is undefined and erroring.

This didn't used to be a problem because before we wouldn't trip into the if statement and we wouldn't evaluate it.

But as we are now evaluating this variable (which will sometimes break things), we need to rethink the function

[newswim]

Prior to refactoring to comply with goodparts, there were some vars in odd places, and goodparts wants them always at the top of whatever scope they're in. You're right, and also moving them may have caused some of the evaluation order breakage.

[samhstn]

On master changing the compile_templates function should comply with goodparts and doesn't break the tests:

function compile_template(template_name, type) {
  var filepathWithoutExt = process.env.TEMPLATE_DIRECTORY + '/' + template_name;
  var filepath = path.resolve(filepathWithoutExt + '.' + type);
  var template = !COMPILED_TEMPLATES[template_name+'.'+type]
    ? fs.readFileSync(filepath, 'utf8')
    : '';
  var compiled = Handlebars.compile(template);

  // check if the template has already been opened
  if(!COMPILED_TEMPLATES[template_name+'.'+type]) {
    COMPILED_TEMPLATES[template_name+'.'+type] = compiled;
  }
  return COMPILED_TEMPLATES[template_name+'.'+type];
}

But it's a little messy

[codecov-io]

Codecov Report

Merging #70 into master will not change coverage.

@@          Coverage Diff          @@
##           master    #70   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          29     31    +2     
=====================================
+ Hits           29     31    +2

Impacted Files	Coverage Δ
lib/index.js	100% <100%> (ø)	✅

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62f7ad4...b900ce6. Read the comment docs.

[newswim]

I had to disable a couple of eslint rules, namely no-console before the error check of ses.sendEmail, and vars-on-top to handle the immediate reassignment of filepath.

We could probably get rid of the console exception by throwing a new Error object instead. I'm not sure how to handle that reassignment.

[samhstn]

I think logging the ses error is fine and using eslint-disable no-console here is ok.

I've rewritten the compile_templates function addressing the vars-on-top rule.

@newswim let me know what you think

[newswim]

Looks good! It's also a bit more readable.

[samhstn]

@newswim Do you think we should also remove the jshint dependecy (and it's baggage) and change the pre-commit hook to use goodparts instead of jshint?

[newswim]

Good call! I'll remove it

[samhstn]

Looks ready to me. @nelsonic Could you have a quick look and merge if your happy.

[iteles]

You guys 😍 Thank you for all the work that's gone into this!

[nelsonic]

Do we no longer need to configure the AWS_REGION before instantiating the SES client? ❓

[nelsonic]

can AWS_REGION be configured down here after SES has been instantiated?
(good ol OOP, hey...?) 😆

[nelsonic]

thanks for tidying up this error throwing. 👍

[nelsonic]

@newswim this looks so much better!! 😍
(thank you so much for making the time to do this refactor. it's textbook!) 🥇
@Shouston3 thanks for remote-pairing / peer-reviewing the refactor. ❤️ ✅ 🚀
Merging. 👍

[nelsonic]

@newswim the latest version on NPM sendemail@3.1.0 contains your changes! thanks again! ❤️