migrate to typescript & node.js-compatible es modules

[zackschuster]

continuation of #241

• port to node.js-compatible es modules
• port to typescript
• fix tests
• configure cjs & esm builds
• set up main + conditional exports to support node 10/12

[zackschuster]

added a test workflow to master so this can block on test failures.

[zackschuster]

@eleith took a week-long break to get fresh eyes. if i can't get it cracked open this weekend i'll ping you to ask for help.

[zackschuster]

quick update: i've been too distracted by shinies, and no further work has been done. next update will be when i actually do something worth commenting on.

[eleith]

i'll look at the failing test this weekend and see if anything pops up on my end

[zackschuster]

that would be great, thank you :)

[eleith]

@zackschuster i traced back the failure and have been able to isolate it and determine the root cause

1. it only happens when you run the very large text message test and any other test after it
2. with debug on, you can see a 221 Bye is received as a response on any test after the very large text message test is successfully run.

i believe this line https://github.com/eleith/emailjs/blob/master/smtp/client.js#L143 to be the cause of the 221 Bye. we are running a poll, to keep the connection open and continue sending any other email that was pushed through.

however, the very large text message is so large, that when it finishes, the timer hits and the connection is closed. however, the following message is already trying to send, so upon sending down a command it was in the middle of, it gets a 221 Bye and it interprets that as an error.

if you change the timeout to 5000 you can get all the tests to pass (though, this is more of a kludge than a solution).

ideally, we restart the timer after the last successfully sent message, to avoid this race condition. thoughts?

[zackschuster]

@eleith good catch! i never would have thought to change the poll timer. i played with that setting & found 2300 is the reliability threshold on my machine.

honestly i'd feel very uneasy pushing a timer change. to me that signals some sort of huge performance hit in the library. whether that's due to e.g. running things through ts-node rather than a precompile step or a mistake i made during the conversion process, i couldn't tell you out-of-hand. but if we pushed that to production as-is we might be breaking things downstream.

[eleith]

on master, i tuned 1000 to 1 and all the tests still pass, so yes, i think this is a regression in this branch somewhere. will try and poke further to find out where, since at first glance, didn't seem like there was a logic change.

[zackschuster]

i'll do another pass as well. there's also a bunch more dependency updates so i'll look into that.

at the very least, once we get things sorted here i'll be looking into adding benchmarks to prevent things like this from reoccurring.

[eleith]

if we change this

_senddone(err: Error | null, stack: MessageStack) {                                                                                                                                        
      this.sending = false;                                                                                                                                                                    
      stack.callback(err, stack.message);                                                                                                                                                                                                                                                                                                                                 
       this._poll();                                                                                                                                                                                                                                                                                                                                                               
    }

to

_senddone(err: Error | null, stack: MessageStack) {                                                                                                                                        
      this.sending = false;                                                                                                                                                                    
      stack.callback(err, stack.message);                                                                                                                                                      
     if (err) {                                                                                                                                                                               
       this._poll();                                                                                                                                                                          
     }                                                                                                                                                                                        
    }

then all tests pass.

i'm still trying to wrap my head around it though. fyi.

[zackschuster]

according to profiles, we were spending a lot of time in regexes on the messages... in testing dependency mailparser (see file isolate-0x1045d7000-38692-v8.txt: emailjs-profiling.zip)

after tracing back the code, it turns out the library was dynamically adding links to the html by default. disabling link generation (which isn't documented in the header files — how fun!) got everything working again.

[zackschuster]

@eleith one million tada emojis

[eleith]

@zackschuster great work. this was a big one, but with the attention to detail and keeping unit testing in place, it feels like a good move forward, even if there is a small chance of regression. i think the win of the types and a cleaner API makes it worth it.

[eleith]

we are introducing breaking changes with the new API. so either the API needs to be updated and this version bumped to 2.2.1 or we need to bump this up to 3.0

[zackschuster]

given the sheer churn involved with this PR, i'm strongly for semver-major

[zackschuster]

re: the API changes — the ESM export should to me still be considered experimental, since we can't test it until userland catches up to the implementation. there might in fact be serious bugs with the code that we just won't see until we have a story for running it against tests. [edit: i remembered npm link; i'll give that a shot in a minute]

i'm also worried i didn't get the conditional exports correct. according to node's docs we should be good, though i did just now add the "type": "module"flag to be sure (i had taken it out before when i thought it might have something to do with testing breaking).

[eleith]

let's bump to 3.0

[zackschuster]

i tested conditional exports with yarn link & everything resolved correctly. i also diffed the bundles & they're practically identical. based on all this i think the ESM export can be considered "stable".

[eleith]

@zackschuster looks good to me with the version bump. anything else?

[zackschuster]

@eleith i think this is ready! 🤞

[eleith]

⛵

[eleith]

@zackschuster lgtm. i'll let you merge when ready.