doc: add conditional export + nested ESM example #33830
Conversation
Such usage was not entirely clear from the doc so adding an example to the dual package section.
lundibundi
added
doc
| "require": "./index.cjs" | ||
| }, | ||
| "./lib": { | ||
| "require": "./lib" |
There was a problem hiding this comment.
Why would this path only be available to require and not import?
There was a problem hiding this comment.
To be able to preserve old behavior of allowing to require files from i.e. lib (and therefore making this backward compatible for libraries which need this) but going forward (with import right away and require on next major release) make use of encapsulation and only expose what's explicitly exposed with exports.
There was a problem hiding this comment.
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.
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.
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 (wrapper.mjs, index.cjs). It makes it very clear what is going on. I'm not sure the documentation for nested conditions is very clear in this respect. It may also be the case that the example is not necessarily using a wrapper module and this is why it is not explicitly named as such. Anyways, I like these kinds of PRs and hope to see more like them. :)
There was a problem hiding this comment.
👋 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 esm.md already as a followup to my ambiguity concern in #33812 (comment) and could make such a change as a separate commit in that PR if it seems like a good idea. LMK
There was a problem hiding this comment.
So, we've been having some more discussions and it is turning out to look like using the "node" condition while also setting a "default" condition may actually be a good best practice going forward after all. For more background see eg rollup/rollup#3634 (comment).
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 "node" condition while always setting a "default" and then only providing "browser" when the build very specificlly is browser-specific will be the way to go.
There was a problem hiding this comment.
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.
I don't think we should let experimental version compatibility affect forwards compatibility.
There was a problem hiding this comment.
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 node/modules about this.
It is tentatively looking though like advising a nested "node" condition while always setting a "default" and then only providing "browser" when the build very specificlly is browser-specific will be the way to go.
+1
I don't think we should let experimental version compatibility affect forwards compatibility.
There's a reason why the documentation is versioned. Things change. :)
Co-authored-by: Derek Lewis <DerekNonGeneric@inf.is>
| // while preventing to `import 'package/lib/file.js'`. | ||
| { | ||
| "main": "./index.cjs", | ||
| "exports": { |
There was a problem hiding this comment.
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.
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.
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.
Yep, looks like it doesn't add up much to the docs (see discussion here), closing.
Such usage was not entirely clear from the doc so adding an example to
the dual package section.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes/cc @nodejs/modules-active-members @nodejs/documentation