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

Feature/esm export #644

Closed
wants to merge 3 commits into from
Closed

Conversation

ckcr4lyf
Copy link

A bug in either node/cjs lexer causes ES6 imports of CJS to fail in some shared environments (steps to reproduce are including in the issue I made here -> nodejs/node#42222)

Although this should ultimately be "solved" in node itself, I was wondering if this ESM export is a potential step dotenv is willing to take.

I'd opened a discussion (Related to this issue - #640), but didn't get any feedback, so thought an MR might be the next step.

This does fix the memory issue for me, when I install dotenv from my fork.

@motdotla
Copy link
Owner

Thanks for the PR @ckcr4lyf. One concern we have with this is maintaining 2 files that essentially implement the same thing here.

Thoughts on that?

@ckcr4lyf
Copy link
Author

I agree, this MR specifically probably isn't the best solution since it leads to duplication with manual copy paste, and maintaining changes as such.

In some of my research, I came across tsup which can bundle the same typescript source into both ESM and CJS.

However considering the size & popularity of dotenv, I was guessing it's been a conscious decision to not introduce compilation from typescript (though it is already a dev dependency). If you are open to the idea, I would be happy to explore a typescript source based build step which leads to producing both esm and cjs form a single source file!

@motdotla
Copy link
Owner

Your guess is right. We'd rather not get into a build process. Copy/paste isn't the worst thing.

We'll let this sit right now and will consider it over the next few weeks.

Anyone else please chime in with your thoughts as well.

@ckcr4lyf
Copy link
Author

Btw: Got feedback from node team - they say this is "intended behavior" - i.e. the lexer for using CJS in ESM reserves a lot of memory, which means in restricted ulimit environments it will not run - nodejs/node#42222 (comment)

So I think at the moment, for anyone using dotenv in a "pure ESM" project (i.e. excluding dependencies), that needs to be run in shared environments with ulimit restrictions, an ESM export fork is required.

@ckcr4lyf
Copy link
Author

Update: guybedford over at node team added a fallback parser for importing CJS in an ESM package via nodejs/node#43612

So on latest node (v18.6.0) I do not have this issue with dotenv. Since it is an official release, I will close this MR for now, though I do think it would be nice to have an ESM export for dotenv in the future.

@ckcr4lyf ckcr4lyf closed this Jul 14, 2022
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.

2 participants