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

[api extractor] export * from 'package' does not export the api surface of the external #1791

Closed
2 tasks done
dzearing opened this issue Mar 19, 2020 · 10 comments
Closed
2 tasks done
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem

Comments

@dzearing
Copy link
Member

dzearing commented Mar 19, 2020

Is this a feature or a bug?

  • Feature
  • Bug

(Maybe feature request, maybe bug?)

I believe this is a known issue but can't find the bug tracking it.

PackageA:
index.ts exports * from 'PackageB'

Expected in api json:

The api surface from PackageB (and recursively, if PackageB exports * from PackageC.

Resulted in the api json:

export * from 'PackageB';
  • Tool: api-extractor
  • Tool Version: 7.7.1
  • Node Version: 10.16.1
  • OS: Mac
@KevinTCoughlin
Copy link
Member

KevinTCoughlin commented Mar 20, 2020

It might be related to #1474. SPFx was tracking that issue to handle wildcard exports of Fabric from a curated Fabric bundle.

@octogonz
Copy link
Collaborator

octogonz commented Mar 28, 2020

@dzearing I feel like this one is "by design".

When package-a does this:

export * from 'package-b';

...the actual exported surface of package-a is not a duplicated clone of each export from package-b. Rather it is a reexport.

For example, if npm install substituted a different SemVer release of package-b, then the actual API surface might be different. It could have entirely new classes/methods than what we might have cloned during API Extractor rollup.

Also, suppose package-b defines a class with private members;

export class Example {
  private _x: number = 123;
}

You might get a TypeScript error when you do this in the consumer project:

import { Example as E1 } from 'package-a';
import { Example as E2 } from 'package-b';

let x: E1 = new E1();
let y: E2 = x;  // compiler error, types are not compatible

(Let me know if I'm misunderstanding something about this request.)

@octogonz octogonz added the needs more info We can't proceed because we need a better repro or an answer to a question label Mar 28, 2020
@dzearing
Copy link
Member Author

dzearing commented Apr 7, 2020

That's a good point @octogonz. The API surface can shift depending on the point in time the end user npm installed and the dependency's version.

But then, what I'm looking at API extractor to do is to warn the dev about downstream API shifts as soon as we bump versions. I want to know that a shift in one package affects another. I'm not necessarily expecting it to be forever perfect, but at least at a point in time a "complete" picture of the API surface.

@octogonz
Copy link
Collaborator

octogonz commented Apr 7, 2020

I see, so this is really about API reporting, not about rollups or documentation. And really about detecting breaks caused indirectly by dependencies.

It makes sense as a feature. But I think we would not want to clutter up package-a.api.md with a bunch of declarations from an external project. Their signatures aren't directly under your control, and a loose SemVer range can make the signatures unpredictable. Thus this seems like qualitatively different information that maybe belongs in its own file.

What if we introduced a setting like "reportReexportedDependencies": true? This would cause API Extractor to write a secondary report file package-a.deps.md alongside package-a.api.md. For each case where your API does export * from 'package-b'; then we would essentially invoke API Extractor separately on package-b and dump the result into package-a.deps.md. Later down the road we could add a setting for also dumping types that are referenced by not reexported (e.g. base classes).

The concept would be:

  • package-a.api.md is my API.
  • package-a.deps.md is all the external stuff that could break my API.

Does that approach make sense to you?

@octogonz octogonz added effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem and removed needs more info We can't proceed because we need a better repro or an answer to a question labels Apr 7, 2020
@dzearing
Copy link
Member Author

dzearing commented Oct 8, 2021

@octogonz wow time flies!

So, yes that makes sense completely. We should separate what a package actually defines from what it simply re-exports. I think a flag like that would be great, and if it detects errors, like duplicated exports or removal of named exports, even if just a point in time detection, errors thrown would be very welcome.

I've come to my senses on export *. It's nonsense. Took me long enough, you were right.

Even when your libraries are in the same mono-repo, even when you follow semver correctly, even when you're careful to avoid breaking your contact, you can still end up adding named exports to multiple sub-libraries completely validly, and introduce export duplication in the suite package. This can result in conflicts downstream.

I think maybe a lint rule enforcing no export * from 'library' might be a better approach to this, with an auto-fix to expand to explicit named exports. This would enforce there are no collisions or mishaps at the suite level and would make it more explicit for a suite package to conform to semver (e.g. no implicit expansion of the contract.)

@octogonz
Copy link
Collaborator

octogonz commented Oct 9, 2021

I think maybe a lint rule enforcing no export * from 'library' might be a better approach to this, with an auto-fix to expand to explicit named exports. This would enforce there are no collisions or mishaps at the suite level and would make it more explicit for a suite package to conform to semver (e.g. no implicit expansion of the contract.)

I'm in favor of that. 👍👍 Even if people sometimes need export * from 'library', it would be useful simply to make them suppress a lint rule, and hopefully read some docs that explain why this practice is error-prone.

I need to add another rule to @rushstack/eslint-plugin for @zpinter, so I'll see if I can include this one with that work. It should be pretty easy to implement.

@dzearing
Copy link
Member Author

Made a proposal here: typescript-eslint/typescript-eslint#3992

@dzearing
Copy link
Member Author

dzearing commented Oct 12, 2021

Turns out this can be linted using this:

    "no-restricted-syntax": [
      "error",
      {
        "selector": "ExportAllDeclaration[source.value=/^[^.]{1}.+$/]",
        "message": "No exporting star from external packages, {insert explanation}"
      }
    ]

@dzearing
Copy link
Member Author

We implemented no-export-all rule here:

https://github.com/microsoft/rnx-kit/tree/main/packages/eslint-plugin

@dzearing
Copy link
Member Author

I'm going to close this as I don't think there's additional actionable work for api-extractor on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem
Projects
None yet
Development

No branches or pull requests

3 participants