search

eleith / emailjs

html emails and attachments to any smtp server with nodejs
MIT License
2.19k stars 230 forks source link

Change fs import to drop need for esModuleInterop #296

Closed CarsonF closed 3 years ago

CarsonF commented 3 years ago

It is only this one import that required esModuleInterop. This one change would allow everyone downstream to not require it.

eleith commented 3 years ago

i like named imports. both for consistency and personal preference. thank you both, i like the direction of the PR.

CarsonF commented 3 years ago

I was going for the smallest change possible since those PRs are easiest to review. But I also prefer named imports. Although I do agree that fs functions in particular are terse and ambiguous without the namespace.

I wasn't aware of star imports having a detriment on emitted code. The roll-up diff seemed very minimal, if any. Curious to hear more about this.

I'm still down to do named imports. I'll patch in the gist in the AM. Thanks guys!

zackschuster commented 3 years ago

to be clear, i'm not sure what the impact is. my first concern is that, for ESM users, we could be changing the cache target from Module._cache to require.cache, which could cause serious headaches downstream. my second concern is that build tools consuming the .mjs might re-emit incorrectly; i've seen the import * as fs from 'fs'; pattern end up being converted to something nasty like fs.fs.open. named imports — which we all seem to agree is the best path forward — neatly sidestep those concerns.

looking forward to your update. as soon as i see it, i'll merge & prepare a release PR. 😄

CarsonF commented 3 years ago

Hope you don't mind, I took a couple more liberties while renaming, like early negative returns to reduce indentation a little bit.

zackschuster commented 3 years ago

looks good & tests pass. i just have one nit & then we can merge 😄

zackschuster commented 3 years ago

Thank you! 😄