Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change fs import to drop need for esModuleInterop #296

Merged
merged 2 commits into from
Aug 28, 2021

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Aug 24, 2021

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

Copy link
Collaborator

@zackschuster zackschuster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

While this passes tests and represents a UX improvement, I'm strongly -1 on using the import * as pattern for builtin modules. It changes the emit in a way where I become unclear if it has runtime implications; that pattern is recommended for CJS modules, but we are meant to act as if the builtins are first-class citizens of the ESM world — import fs from 'fs'; is what we're "supposed" to write. It's also by far the dominant pattern in literature about how to write these statements.

All that being said, I do think the UX improvement is too good to pass up, so I'd like to propose we switch to using named imports instead, which matches the code style for the rest of the library. I've gone ahead and prepared a gist with the appropriate changes. If you're ok with that, please add it to the PR and I'll merge ASAP; otherwise, we can discuss it further. @eleith please let me know if you have any thoughts on the matter.

Once again, thank you for your effort here. ^_^

(Addendum: In the past, I think I decided to go with the default import as the alternative was more verbose, and the function names are pretty generic without the fs prefix — open and read already had name collisions with internal helper functions, even. This is a pretty minor gripe, though, and the code has proven stable enough that I don't think it's really something to be concerned about. If it becomes an issue — which I highly doubt — we can always just alias the names, e.g. open as fsOpen. But again, I don't expect that to be necessary.)

@eleith
Copy link
Owner

eleith commented Aug 26, 2021

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

@CarsonF
Copy link
Contributor Author

CarsonF commented Aug 27, 2021

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
Copy link
Collaborator

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
Copy link
Contributor Author

CarsonF commented Aug 27, 2021

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

smtp/message.ts Outdated Show resolved Hide resolved
@zackschuster
Copy link
Collaborator

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

@zackschuster zackschuster merged commit 502196e into eleith:main Aug 28, 2021
@zackschuster
Copy link
Collaborator

Thank you! 😄

@CarsonF CarsonF deleted the ts-no-esm-interop branch August 28, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants