Code clean-up tasks

[k-mahoney]

Ticket to track assorted, lingering code clean-up tasks, in preparation for future open source plans.

• Merge linter https://github.com/mapbox/dispatch/pull/36
• Add Travis
• Abstract dispatch-incoming GitHub issue creation to function
• Abstract dispatch-incoming Slack alert post to function
• Normalize styling
• Normalize test structure
• Add in-line function documentation
• Expand tests in dispatch-incoming
  • Tests for decrypt failure behavior
  • Add tests for error handling in self-service events
  • Add tests for error handling in PD events

/cc @oliikit @ianshward

[alulsh]

I did a code review of Dispatch with an eye for open sourcing and also am doing some code clean up tasks. Here's what I'm doing and tracking. This is not necessarily a complete list, I'll either update this comment or add new comments as I work on things:

• Add package-lock.json (https://github.com/mapbox/dispatch/issues/87) - done in https://github.com/mapbox/dispatch/pull/94/commits/8276be9d038289bf91b3c7f94f964fd4fa7bb121
• Remove Node 4 from Travis (https://github.com/mapbox/dispatch/issues/86) done in https://github.com/mapbox/dispatch/pull/94/commits/989b601fb02698ff2f6b58dbc2b4fd93882f3dc9
• Remove .slugger.json - done in https://github.com/mapbox/dispatch/pull/94/commits/89a47d0f18cc6b152afc1541af14dd40cbce9aa8
• Remove all vars and replace with let or const - done in https://github.com/mapbox/dispatch/pull/94/commits/0194aa49717649de511eef16f9d0b0cdca47a6bb
• Make sure each JavaScript file has use strict - done in https://github.com/mapbox/dispatch/pull/94/commits/0194aa49717649de511eef16f9d0b0cdca47a6bb
• Add standard Mapbox license (file and in package.json) - done in https://github.com/mapbox/dispatch/pull/94/commits/1b211b49f385cd6ec7e51bf2197bf2a22c0c8a88
• Switch to strict equality (=== instead of ==)

[k-mahoney]

I'm not sure I understand the reasoning behind having a package lock here.
Rarely see it used, much less committed to a repository.

[alulsh]

@k-mahoney see https://github.com/mapbox/dispatch/issues/87 and https://github.com/mapbox/dispatch/issues/84. A package-lock.json is supposed to be checked in to source code.

[alulsh]

Oops https://github.com/mapbox/dispatch/pull/85 explains it better, but essentially a minor bump in nock broke the tests on master. With a package-lock this would've been avoided.

[k-mahoney]

Alright update, why would we commit this huge dependency tree rather than just pinning versions in the package.json?

[alulsh]

@k-mahoney - You can run npm update if you want to check for updates to dependencies. The package-lock.json helps to ensure that what you're running locally on your dev machine is the same as what gets deployed. It also makes sure that what's on staging is the same as what gets deployed to master.

Not using a package-lock.json cost 2 members of our team 2 days of work to fix a bug that wasn't our fault, so I am biased towards using one to prevent this from happening again.

[k-mahoney]

Wow, today I learned NPM's version pinning doesn't actually work and their idea of a bandaid is dumping the entire dependency tree.

A dependency of one of your dependencies may have published a new version, which will update even if you used pinned dependency specifiers (1.2.3 instead of ^1.2.3)

Thank you for your patience @alulsh - carry on 😬

[alulsh]

Merged and deployed https://github.com/mapbox/dispatch/pull/94 to staging and production tonight. This was my top priority clean up + open source prep.

The other clean up work I would do would be fixing + improving error messages, but I see this as a lower priority than updating our docs for external audiences as well as other bug fixes and feature improvements.

[k-mahoney]

In progress in https://github.com/mapbox/dispatch/pull/113.

[k-mahoney]

These are complete in #113.