Skip to content
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: update conditions, add "deno" and "types" #40708

Closed
wants to merge 15 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Nov 3, 2021

This adds two new conditions to the endorsed "exports" conditions list in the Node.js documentation.

  1. "deno" - as of https://github.com/denoland/deno/pull/12424/files#diff-0523983fa24098bdaddcdb01d5f3771335b9e564df0c81d4b79101e75a108718R34, Deno now supports matching the "deno" condition when resolving using its Node.js compatibility resolver, allowing packages to define variants specifically for Deno. This makes the text in the Node.js documentation incorrect as since now the implementation exists we can correspondingly endorse its usage.
  2. "types" - Angular have adopted this condition in the "exports" field as described in https://rc.angular.io/guide/angular-package-format#exports, explicitly calling out this pattern for typing can help move typing forward for the "exports" field.

It also reorders conditions by specificity and clarifies some of the definitions including:

  • Browser is now defined strictly to be browser
  • Development is strictly defined to imply debugging context

These could possibly be split out into separate PRs for further discussion if there are any objections to any of the specific conditions listed in this PR.

@nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 3, 2021
@guybedford guybedford changed the title doc: endorse "source" and "deno" conditions doc: endorse "source", "deno" and "types" conditions Nov 3, 2021
@guybedford
Copy link
Contributor Author

I've amended this PR to also include a "types" condition //cc @weswigham would be great to get your feedback on this as well.

doc/api/packages.md Outdated Show resolved Hide resolved
Co-authored-by: Rich Trott <[email protected]>
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@guybedford guybedford changed the title doc: endorse "source", "deno" and "types" conditions doc: endorse "deno" and "types" conditions Nov 3, 2021
@guybedford
Copy link
Contributor Author

I've removed the "source" condition for now, and also included a clarification that the "development" condition is primarily for debugging context use cases.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@guybedford guybedford changed the title doc: endorse "deno" and "types" conditions doc: update conditions, add "deno" and "types" Nov 4, 2021
doc/api/packages.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Nov 5, 2021

It seems really strange to me to explicitly endorse a private company’s product in node’s documentation.

@guybedford
Copy link
Contributor Author

It seems really strange to me to explicitly endorse a private company’s product in node’s documentation.

We originally discussed having a conditions registry being maintained outside of Node.js, but then made the decision to maintain the list in the Node.js documentation for now.

No one has so far expressed any issue about listing "deno" here, but if there is further concern we could discuss alternatives.

@ljharb
Copy link
Member

ljharb commented Nov 5, 2021

The other conditions are all broad community standards, like browsers or types, or node things themselves.

I don’t think it’s really appropriate for node to claim a “registry” when node isn’t enforcing anything like that - it seems better for each tool to describe the conditions that tool supports.

@guybedford
Copy link
Contributor Author

@ljharb I personally feel quite strongly that especially given the risks of confusion we should be putting together more guidance not less with regards to usage of the "exports" field, and that maintaining a well-defined list of conditions most JS developers should know about is part of that effort.

Your argument here seems to be on the grounds of "appropriate", which seems quite ill defined. Can you try to explain more clearly what exactly you disagree with about Node.js mentioning the "deno" condition? IMO this isn't a case of companies competing, but platforms coordinating in the shared interest of the user.

@ljharb
Copy link
Member

ljharb commented Nov 5, 2021

I agree with you that those resources should exist, but it feels like the sort of thing the community should be deciding - node’s docs are authoritative and supposed to be primarily about how to use node.

There’s no way imo that “most” JS developers “should” know about a lot of the options for this list - they’re things that a subset of developers, and often a small subset, will want to know about. Package authors targeting a non-node platform generally will be familiar with that platform’s docs, as will tooling authors - who else needs to know about possible exports conditions?

doc/api/packages.md Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

I've added a commit to clarify that the "conditions definitions" Node.js docs section is an entire documentation section dedicated to maintaining a list of conditions defintions of conditions not directly implemented in Node.js as a community coordination help.

In the process I removed some redundant text and also realised the node-addons condition description had been incorrectly placed into this section when that is a Node.js condition so does not belong in the definitions registry section.

To help further clarify this docs section, we could also consider renaming the title from "condition definitions" to "community definitions" to be clear that this is a coordination effort and not to do with documenting Node.js.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'm generally ok with the current wording.

I would still see if anybody from the @nodejs/tsc object to this.

doc/api/packages.md Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

@weswigham in the TSC meeting today we discussed that it would be good to verify that there is implementer agreement with these conditions definitions. Can you perhaps explicitly confirm / approve you are happy with the "types" definition as written in this PR?

@weswigham
Copy link

Yes with one caveat - it's fine for different conditions of an entrypoint to have different APIs (a la module vs require) - they just need separate types entries then; which is one of the first times people may be exposed to nested conditions. Eg,

{
  "exports": {
    ".": {
      "types": {
        "module": "./types/wrapper.d.mts",
        "default": "./types/entrypoint.d.ts"
      },
      "module": "./wrapper.mjs",
      "default": "./entrypoint.js"
    }
  }
}

And, in fact, you typically must provide a separate definition for each version of an entrypoint in such a scenario (to at least capture the differing format data).

@guybedford
Copy link
Contributor Author

Thanks for the confirmation @weswigham. Point taken re the dual typing - to avoid getting too much into the details I've removed the note about singular typing interface requirement for now. If you have a resource on this that can be linked to in future for further guidance please do share it here.

@GeoffreyBooth
Copy link
Member

If you have a resource on this that can be linked to in future for further guidance please do share it here.

This would perhaps be a great thing for TypeScript to document, and we could link to that.

@guybedford
Copy link
Contributor Author

This PR no longer has a block and as per the TSC discussion we have now had implementer feedback from both Deno and TypeScript on these conditions as well. Does anyone feel any further requirements should be met to land this PR? If not I will aim to land in the coming days.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ljharb
Copy link
Member

ljharb commented Nov 19, 2021

@guybedford can you link to that implementer feedback?

@guybedford
Copy link
Contributor Author

@ljharb see #40708 (comment) and #40708 (comment).

guybedford added a commit that referenced this pull request Nov 21, 2021
PR-URL: #40708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@guybedford
Copy link
Contributor Author

Landed in e31d1cb.

@guybedford guybedford closed this Nov 21, 2021
@guybedford guybedford deleted the doc-new-conditions branch November 21, 2021 00:42
targos pushed a commit that referenced this pull request Nov 21, 2021
PR-URL: #40708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
PR-URL: #40708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #40708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.