Replace moment.js with custom date generator
#223
Conversation
| "emailjs-mime-codec": "^2.0.5", | ||
| "moment": "2.20.1", | ||
| "starttls": "1.0.1" | ||
| "emailjs-mime-codec": "^2.0.7", |
There was a problem hiding this comment.
before there was a yarn/package.json lock file, i had some experience with even minor bumps introducing bugs that impacted deployments that did not intend to have anything upgraded.
now with locks, i don't see that happen as much. what are you thoughts on it? for dev dependencies, i don't think it matters as much, but for direct dependencies, it's worth thinking about.
since you removed moment and changed startls to be 'compatible' they are now all consistent. so that is good.
let me know what you think and we can run with this or to be safe we can change them all to be locked to one version.
There was a problem hiding this comment.
personally my stance is, now that lockfiles are normalized, an end user can and should be able to ensure their builds are reproducible without libraries needing to pin themselves -- in fact, libraries should avail themselves of all the latest versions of libraries they can take without breaking tests. that's the best way (in my mind) to ensure the most secure and bug-free code is shipped.
that being said, latest version of node 6 ships with npm 3.10.3, and package-lock wasn't introduced until npm 5 -- i wouldn't want to dismiss people in that situation out-of-hand, knowing that e.g. joyent will still be on 0.10 for some time.
i wonder, though, if people in that situation would even bother touching their dependencies, especially if the next release is cut as semver major. it might be a moot point altogether, in which case i fall back to my original argument.
smtp/message.js
Outdated
|
|
||
| let counter = 0; | ||
|
|
||
| function getRFC2822Date(date = new Date(), useUtc = false) { |
There was a problem hiding this comment.
what about pulling these out into a utility file like RFC2822.js?
i don't feel too strongly about it, but since you are exporting these functions, it would provide an opportunity to extract them an expose the functions on a file that is more granular focused.
There was a problem hiding this comment.
I moved these to a new file, ./smtp/date.js. also moved the tests to a new file & split them by purpose (UTC vs. non-UTC).
Test assertions use output from the original moment.js code in
require('./smtp/message').create.Also added a lockfile & bumped all the "safe" dependencies to latest.