Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
doc: add conditional export + nested ESM example #33830
Changes from 1 commit
7e8175e
919d76f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Why would this path only be available to
require
and notimport
?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.
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 (withimport
right away andrequire
on next major release) make use of encapsulation and only expose what's explicitly exposed withexports
.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.
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 comment
The 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 comment
The 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 (
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.
Choose a reason for hiding this comment
The 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 …
https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_writing_dual_packages_while_avoiding_or_minimizing_hazards
… 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. LMKThere 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.
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.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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
node/modules
about this.+1
There's a reason why the documentation is versioned. Things change. :)