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
doc: add conditional export + nested ESM example #33830
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -689,6 +689,31 @@ stateless): | |
} | ||
``` | ||
|
||
Another variant to also preserve backward-compatibility of `require`ing some | ||
files from the package that were previously available is to combine [Conditional | ||
Exports][] with [Nested conditions][] as follows: | ||
|
||
<!-- eslint-skip --> | ||
```json | ||
// This will allow to: | ||
// `import 'package'` | ||
// `require('package')` | ||
// `require('package/lib/file')` | ||
// while preventing to `import 'package/lib/file.js'`. | ||
{ | ||
"main": "./index.cjs", | ||
"exports": { | ||
".": { | ||
"import": "./wrapper.mjs", | ||
"require": "./index.cjs" | ||
}, | ||
"./lib": { | ||
"require": "./lib" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would this path only be available to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be able to preserve old behavior of allowing to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit weird to encapsulate only for ESM and not for CommonJS. I think the pattern we'd more likely encourage would be to expose the same public API for both, even if it means exposing more than you'd like, until the next semver major change when you restrict it for both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense, having API parity should probably be more important in this case. I understand the importance of promoting good patterns in our docs, so I guess we can close this? Or maybe there is a better place to put/adapt this example in the doc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There appears to be a good example of combining conditional exports and nested conditions. https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_nested_conditions One thing I liked about this example (and have thumbs upped it) is the naming scheme ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👋 Sorry to bring this up again, but does anyone else in this PR thread think that it would be a good idea to replace the example shown in … https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_nested_conditions … with one of the examples (or a slight derivative) of one of the recommended dual-mode packages shown in … … with the justification being that it further promotes well-established recommendations? I've revised a couple of problem areas in the conditional exports section of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, we've been having some more discussions and it is turning out to look like using the We're still having these discussions though so I would wait just a little for things to settle. It is tentatively looking though like advising a nested There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that there's a number of versions of node that either ignore, or warn on, the "node" condition :-/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should let experimental version compatibility affect forwards compatibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, so I won't be adding a commit for this in my PR. Let me know if you want me to open an issue
+1
There's a reason why the documentation is versioned. Things change. :) |
||
} | ||
} | ||
} | ||
``` | ||
|
||
##### Approach #2: Isolate State | ||
|
||
A `package.json` file can define the separate CommonJS and ES module entry | ||
|
@@ -1807,6 +1832,7 @@ success! | |
[ECMAScript-modules implementation]: https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md | ||
[ECMAScript Top-Level `await` proposal]: https://github.com/tc39/proposal-top-level-await/ | ||
[ES Module Integration Proposal for Web Assembly]: https://github.com/webassembly/esm-integration | ||
[Nested conditions]: #esm_nested_conditions | ||
[Node.js EP for ES Modules]: https://github.com/nodejs/node-eps/blob/master/002-es-modules.md | ||
[Terminology]: #esm_terminology | ||
[WHATWG JSON modules specification]: https://html.spec.whatwg.org/#creating-a-json-module-script | ||
|
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.
Perhaps I am missing something, but where is an example of a nested condition below, I don't see it.
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.
Oops, looks like I misinterpreted nested conditions in our doc as having conditions not-at-top-level of exports, but that's not true. Will remove that remark.
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.
if that is the case there are already a bunch of examples of conditional exports... I'm not 100% that this adds explicit value to the docs.
Can you expand a bit more on what makes this example unique
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.
Yep, looks like it doesn't add up much to the docs (see discussion here), closing.