-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 support for named imports in ESM #1440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
So fast! 🚀 |
"files": [ | ||
"index.js", | ||
"wrapper.mjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a typo, must be esm.mjs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch, thanks! I changed the name half way through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(For interest, I thought wrapper
was a suitable name for a wrapper used with conditional exports and not seen by normal client use. But when I changed to deep import and the name became visible, I wanted something more meaningful.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #1443
Pull Request
Problem
Importing commander as CommonJs into an ECMAScript module only allows using the default import, and can not use named imports.
Related issue: #1284
Solution
Add esm wrapper and use as a deep import:
In the future we will add support so the deep import is not needed by using conditional exports and/or subpaths. Being conservative for now as the functionality is still marked as experimental in node 12. (There have been breaking changes in the support in minor versions of node 12 and 13.)
The
--experimental-modules
option used in thetest-esm
run-script will get dropped by node eventually, but currently allows that test to run locally with node 10 through node 15.ChangeLog