-
Notifications
You must be signed in to change notification settings - Fork 774
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
Add basic ESM support #854
Conversation
The complication is that some |
Currently, not at all. In such a situation, the call to
I was not aware that there are platform-dependent differences in A quick web search yielded no useful information on that problem. Can you point me to some solid documentation on that? Is that a general issue, or version-dependent? On what platform could I check a proposed solution? If this is version-dependent, the question becomes whether that is still an issue post-v12.7.0, since that is the version when the
That is another bag, and needs to be addressed unconditionally. I see these options:
Both approaches have their drawbacks, but I'm gravitating towards the export with custom errors. What do you think? |
It seems the situation for However, I did a bit of testing, and this is not an issue for us, as
import * as fs from 'fs'
console.log('lchmod' in fs) // -> true
console.log(fs.lchmod) // -> undefined
console.log('opendir' in fs) // -> false
console.log(fs.opendir) // -> undefined This behavior is impossible to replicate without |
The only viable option I see to achieve availability-based static exports is building the exports locally, in a post-install script. That would necessitate a rebuild on a node update. Also, specific testing may be somewhat hairy. What do you think? |
Wow, nearly a month; my apologies for letting this go so long without response. In general, I think Long-term, I'd like to consider removing promise polyfilling of |
Here is my current proposal for a way forward; I will plan to implement this unless there's good opposition: #746 (comment) |
Closing in favor of #974; feel free to review there. |
Fixes #746
@RyanZim @jprichardson
I honestly do not see the big challenges mentioned in #746 – I think, apart from documentation, this should do it. It works as intended in a project I am working on. Am I missing something, or can this be merged?
Should node change its ESM interface, that might result in a breaking change here as well. However, that seems rather unlikely and will not happen often if at all. I vote to implement this now and change later if necessary, and mark this experimental as long as the node implementation is marked experimental.