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

Breaking: define an exports field for our public API #13654

Closed
kaicataldo opened this issue Sep 4, 2020 · 23 comments · Fixed by #14706 or #14716
Closed

Breaking: define an exports field for our public API #13654

kaicataldo opened this issue Sep 4, 2020 · 23 comments · Fixed by #14706 or #14716
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features

Comments

@kaicataldo
Copy link
Member

kaicataldo commented Sep 4, 2020

The version of ESLint you are using.

Latest

The problem you want to solve.

Downstream packages rely on ESLint's internal modules, and removing/altering these modules can cause breakages in the ecosystem (example here). A lot of thought has gone into what is and isn't our public API (defined here), but it would be great to be able to enforce that downstream packages don't rely on these modules, since treating every module in our codebase as public isn't practical and would make it extremely difficult to refactor or do feature work.

To be clear, I don't think that our current strategy of defining our public API and then treating all other modules as "private" is an uncommon practice, but I do think this small change could prevent some instances of this from happening in the future.

Your take on the correct solution to problem.

As suggested here, I would like to propose that we define an exports field to a) clearly document what we consider our public API from an importable module standpoint, and b) prevent those using more recent versions of Node.js from importing internal modules not intended for public consumption. For older versions of Node.js, main (which we have defined) would be used as a fallback. You can find more details in the Node.js Package Entry Points docs.

Are you willing to submit a pull request to implement this change?

Absolutely!

@kaicataldo kaicataldo added core Relates to ESLint's core APIs and features breaking This change is backwards-incompatible labels Sep 4, 2020
@ljharb
Copy link
Sponsor Contributor

ljharb commented Sep 4, 2020

When adding this, you can use npx ls-exports path . to verify what exports you're exposing. This can be used to add the field in a nonbreaking way right now as well.

Either way, I'd be happy to review any change around this field for correctness (node 13.0 and 13.1, if supported, require extra consideration in the format, as do a few non-latest minors in 12 and 14); please feel free to ping me.

@nzakas
Copy link
Member

nzakas commented Oct 1, 2020

@kaicataldo Did you mean to link to here?
https://nodejs.org/api/packages.html. The link in the description is to the ESM Docs.

This seems like a simple enough change to prevent any further misunderstandings about what is and isn’t a public API.

@nzakas nzakas added the evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion label Oct 1, 2020
@kaicataldo
Copy link
Member Author

@kaicataldo Did you mean to link to here?
https://nodejs.org/api/packages.html. The link in the description is to the ESM Docs.

That's right - looks like the docs were moved.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 1, 2020

Note as well that all my complaints about breaking changes in non-majors from renaming internal files would go away if internal things were excluded from "exports" :-) (or at least, change to feature requests to expose them explicitly)

@nzakas
Copy link
Member

nzakas commented Oct 7, 2020

TSC Summary: This proposal seeks to add an exports field to package.json that specifically identifies which files are part of the public API. Despite our consistently saying people should not be accessing files directly in the eslint package, people still are, and then complain when we make a change. For new Node.js versions, including exports would ensure our internal file structure isn't exposed publicly.

TSC Question: Shall we accept this proposal and add it to the public roadmap (for v8.0.0).

@nzakas nzakas added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Oct 7, 2020
@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Oct 10, 2020
@btmills btmills added this to Planned in Public Roadmap via automation Oct 10, 2020
@btmills
Copy link
Member

btmills commented Oct 10, 2020

In the 2020-10-08 TSC meeting, we accepted this as a semver-major change for the v8.0.0 release. I've added it to our public roadmap. The next step is for someone to write an RFC outlining which endpoints we need to expose and hopefully identifying which packages currently import private API that will be affected by this change. As we start working on v8.0.0, we can mention this change in a blog post to give those packages plenty of notice.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 10, 2020

@btmills once the choices are made, I’d be happy to volunteer to write the PR to implement it.

@nzakas nzakas added the needs design Important details about this change need to be discussed label Oct 12, 2020
@nzakas nzakas moved this from Planned to Needs RFC in Public Roadmap Oct 12, 2020
@mdjermanovic mdjermanovic added this to Need Discussion in v8.0.0 Apr 7, 2021
@nzakas nzakas added this to Needs Triage in Triage via automation Apr 8, 2021
@nzakas nzakas moved this from Needs Triage to Evaluating in Triage Apr 8, 2021
@nzakas nzakas moved this from Need Discussion to Planned in v8.0.0 Apr 8, 2021
@nzakas nzakas moved this from Evaluating to Waiting for RFC in Triage Apr 8, 2021
@btmills btmills added the help wanted The team would welcome a contribution from the community for this issue label Apr 12, 2021
@btmills
Copy link
Member

btmills commented Apr 12, 2021

We’d like to include this in our upcoming v8.0.0 release if possible. The next step is to write an RFC specifying which portions of our API we want to expose via exports. Is anybody interested in contributing that? The team would be happy to answer questions along the way.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Apr 12, 2021

Once the list of entrypoints exists, i will still happily make the PR to define them.

@bradzacher
Copy link
Contributor

worth noting that @typescript-eslint does a bunch of deep, direct importing of rules so that we can define extension rules. Which I'd assume this change would break?

We could technically go via the root import - but I thought it was nicer to restrict the footprint of the import. Probably makes no difference though as ESLint should have already been required and initialised by the time our rules are imported.

@nzakas nzakas moved this from Planned to Pull Request Opened in v8.0.0 Jun 16, 2021
@nzakas nzakas moved this from RFC Opened to Pull Request Opened in Triage Jun 16, 2021
@nzakas nzakas moved this from RFC Under Review to In Progress in Public Roadmap Jun 16, 2021
Public Roadmap automation moved this from In Progress to Complete Jun 18, 2021
Triage automation moved this from Pull Request Opened to Complete Jun 18, 2021
btmills added a commit that referenced this issue Jun 18, 2021
* New: Add ESLint#getRulesMetaForResults() (refs #13654)

* Update docs

* Update docs/developer-guide/nodejs-api.md

Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>

Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
@bradzacher
Copy link
Contributor

bradzacher commented Jun 18, 2021

note - this should not have been closed yet - the PR only adds one piece of it.

@btmills
Copy link
Member

btmills commented Jun 18, 2021

Thanks for catching this @bradzacher, looks like automation got excited. Reopening and re-adding to project boards.

@btmills btmills reopened this Jun 18, 2021
Public Roadmap automation moved this from Complete to Planned Jun 18, 2021
Triage automation moved this from Complete to Evaluating Jun 18, 2021
@btmills btmills moved this from Planned to In Progress in Public Roadmap Jun 18, 2021
@btmills btmills moved this from Evaluating to Pull Request Opened in Triage Jun 18, 2021
@btmills btmills moved this from Pull Request Opened to Ready for Merge in v8.0.0 Jun 18, 2021
@mdjermanovic mdjermanovic removed help wanted The team would welcome a contribution from the community for this issue needs design Important details about this change need to be discussed labels Jun 21, 2021
Public Roadmap automation moved this from In Progress to Complete Aug 5, 2021
Triage automation moved this from Pull Request Opened to Complete Aug 5, 2021
v8.0.0 automation moved this from Ready for Merge to Done Aug 5, 2021
nzakas added a commit that referenced this issue Aug 5, 2021
* Breaking: Strict package exports (refs #13654)

* Update docs

* Fix linting errors

* Update docs/developer-guide/nodejs-api.md

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update docs/developer-guide/nodejs-api.md

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 2, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features
Projects
Public Roadmap
  
Complete
Archived in project
v8.0.0
  
Done
Triage
Complete
7 participants