[api-extractor] Fix incorrect declaration references for symbols not exported from the package's entry point #3584

zelliott · 2022-08-12T13:57:04Z

Summary

This is a bug fix found while continuing to work on #3552. Consider the following situation:

// internal.ts

export class A {}

// index.ts

import { A } from './internal';

/** @public */
export class B extends A {}

In the code above, there are two files, internal.ts and index.ts. index.ts is the main entry point of the package, internal.ts is a local file within the package.

Before this PR, api-extractor's DeclarationReferenceGenerator constructs the following reference for A: my-package!A:class. This reference is incorrect, it should be my-package!~A:class. A is not exported from the entry point, and so it should use a Locals navigation step.

This PR fixes this bug, and in general cleans up the DeclarationReferenceGenerator._getNavigationToSymbol method by removing a ton of code paths that as far as I can tell are never hit.

Details

Before this PR, DeclarationReferenceGenerator had dependencies on different parts of the Collector. Now, it has a dependency on the entire Collector itself (because it needs to call a method on the Collector).
As mentioned above, this PR removes a ton of logic from the _getNavigationToSymbol method because I could not find scenarios in which they were ever hit. It would be great to run this change against some large actual monorepos before merging it to make sure it doesn't break things.

How it was tested

Test cases existed in api-extractor-scenarios, added one final one, ran rush rebuild, and inspected the .api.json files.

…nexported symbols

zelliott · 2022-08-12T14:06:39Z

apps/api-extractor/src/generators/DeclarationReferenceGenerator.ts

+      // If its parent symbol is not a source file, then use an Exports navigation. If the parent symbol is
+      // a source file, but it wasn't exported from the package entry point (in the check above), then the symbol
+      // is a local, so fall through below.
+      if (!DeclarationReferenceGenerator._isExternalModuleSymbol(parent)) {
        return Navigation.Exports;
      }


In my testing, I could not find a scenario in which any line from 112 to 157 was hit. I could also not find a situation where Members navigation was ever returned. I'm worried I may be missing an entire class of cases here... the original logic was added in the mega-PR #1337.

Maybe this logic was here because previously this method was used to generate the canonical references for API items in the .api.json, but now these references are generated from the .api.json structure itself.

@rbuckton any idea what kind of type declaration input would cover these these code paths?

@octogonz shared some instances where Members navigation was returned, and I'm covering those now. But I still can not find a scenario where any line from 118-157 was hit.

apps/api-extractor/src/generators/DeclarationReferenceGenerator.ts

zelliott · 2022-08-12T14:29:07Z

apps/api-extractor/src/generators/DeclarationReferenceGenerator.ts

@@ -99,68 +87,48 @@ export class DeclarationReferenceGenerator {
    );
  }

-  private static _getNavigationToSymbol(symbol: ts.Symbol): Navigation | 'global' {
+  private _getNavigationToSymbol(symbol: ts.Symbol): Navigation {


Here's an interesting canonical reference question: Consider the following example:

// index.ts (entry point of "my package") import * as n from './internal'; export function someFunction(): n.SomeType { return 5; } // internal.ts export type SomeType = number;

What should the canonical reference of SomeType be? Today...

DeclarationReferenceGenerator produces the following: my-package!~SomeType:type (after this PR)

whereas api-extractor-model produces the following: my-package!~n.SomeType

api-extractor-model produces the latter because the "synthetic" AstNamespaceImport for n makes its way into the .api.json.

I feel like the correct canonical reference in this case is my-package!~SomeType:type, because the namespace n is really an implementation detail of how index.ts imports from internal.ts, and not part of SomeType itself. There could be the following code in internal.ts:

export function someOtherFunction(): SomeType { return 6; }

and it would be odd for someOtherFunction's reference token for SomeType to include the synthetic namespace n. I'd expect for it to be my-package!~SomeType:type.

Regardless, I don't think we should solve this in this PR, just something to think about.

Hmm.... A central principle of API Extractor is that it interprets declarations, imposing a semantics that is not technically present in the source code. This is what justifies rolling up declarations into a different file from where they were declared, discarding some .d.ts files entirely (because they aren't reachable from the entry point), and renaming API items to avoid naming conflicts.

Your question is really about what semantics should be applied to an unexported declaration.

I'm a little sketchy on the grammar, but I think the physical location would be described as my-package/lib/internal!~SomeType:type.

But generally API Extractor's model is that of the .d.ts rollup. Physically in the .d.ts rollup, the location would be my-package!~SomeType:type.

However if there are naming collisions, this could become something arbitrary like my-package!~SomeType_3:type.

Logically, the human intent of import * as n is to move this declaration inside n which would better be described as my-package!n~SomeType:type. The reason it does not get placed there was technical: if they also did import * as n2 then its ambiguous which one is the "real" definition and which one is the alias. With more sophisticated rollup logic, we could solve this problem in the future, and then my-package!n~SomeType:type maybe would be correct.

I guess the choice boils down to requirements:

Is our priority that the canonical reference should help a tool to find the declaration in the .d.ts files? If so then it should probably be a physical location in the .d.ts rollup. If the .d.ts rollup feature is disabled, then maybe all canonical references should actually be physical locations such as (1).

Alternatively, is our priority that the canonical reference should be a stable identifier, i.e. resistant to getting shuffled around by trivial changes? I think this is the design choice that API Extractor has pursued. In this case I guess we would go with (2) or (3).

octogonz · 2022-08-24T00:39:34Z

apps/api-extractor/src/generators/ApiModelGenerator.ts

@@ -54,13 +54,7 @@ export class ApiModelGenerator {
  public constructor(collector: Collector) {


I ran your build of API Extractor on some monorepo projects and compared the resulting .api.json files with the baseline output. Here's some differences:

(I've deleted some replies, since I was comparing in the wrong direction 😆 )

1. Interesting case with inline type destructuring

This is NOT good API practices. 😄 But it is an interesting edge case:

forgotten-exports.d.ts

export declare type HeadersInitializer = Record<string, string>; export interface IOptions { headers?: HeadersInitializer; }

index.d.ts

import { IOptions } from './forgotten-exports'; export declare const example1: ({ headers: addHeaders }: IOptions) => Promise<void>;

Before this PR, the excerpt for example1 includes:
"canonicalReference": "the-package!IOptions#headers"

After this PR:
"canonicalReference": "the-package!IOptions.headers"

2. Case involving "typeof"

// Resolves to [email protected]\node_modules\webpack\types.d.ts import webpack from 'webpack'; export declare type Example2 = typeof webpack.prototype.inputFileSystem;

Before this PR:

"excerptTokens": [ { "kind": "Content", "text": "export declare type FileSystem = " }, { "kind": "Content", "text": "typeof " }, { "kind": "Reference", "text": "webpack.prototype", "canonicalReference": "!Function#prototype:member" }, { "kind": "Content", "text": ".inputFileSystem" }, { "kind": "Content", "text": ";" } ],

After this PR:

"canonicalReference": "!Function.prototype:member"

I'm not sure either string is really correct. 😋

These examples are really useful! I couldn't find any examples of code generating Navigation.Members navigation steps. I'll take a look at these. 👍

3. Case involving preact

This is not a bug really, more of a curiosity:

import { Context } from 'preact'; export declare const Example3: Context<{}>;

Before this PR:

{ "kind": "Reference", "text": "Context", "canonicalReference": "preact!~preact.Context:interface" },

After this PR:

{ "kind": "Reference", "text": "Context", "canonicalReference": "preact!preact.Context:interface" },

It is an improvement, but I think this canonical reference maybe should not have the extra preact part?

The Context is declared like this:

[email protected]\node_modules\preact\src\index.d.ts

export = preact; export as namespace preact; . . . declare namespace preact { . . . interface Context<T> { Consumer: Consumer<T>; Provider: Provider<T>; displayName?: string; } }

So it is a member of namespace preact but the intended way of importing is really import { Context } from 'preact';. 🤷‍♂️

Across >30 libraries, these were the only weird cases. There were a number of canonical references that are fixed by your PR. Otherwise I didn't see any obvious regressions. 👍

Okay. I think cases 1 and 2 should now have the same behavior as before this PR. Previously, I didn't think it was possible for a members navigation step to be present in an identifier passed to this function, but the cases you shared demonstrated it is possible.

I didn't tackle case 3 because this PR is an improvement, and it seemed like potentially a larger fix. But I agree that the currently behavior isn't necessarily ideal.

# Conflicts: # build-tests/install-test-workspace/workspace/common/pnpm-lock.yaml

… logic change is nontrivial

octogonz · 2022-09-02T21:07:19Z

🚀 This feature was released with API Extractor 7.30.0

Improve DeclarationReferenceGenerator logic to handle more cases of u…

d61f74f

…nexported symbols

zelliott requested review from iclanton, octogonz, apostolisms and D4N14L as code owners August 12, 2022 13:57

zelliott commented Aug 12, 2022

View reviewed changes

apps/api-extractor/src/generators/DeclarationReferenceGenerator.ts Show resolved Hide resolved

zelliott commented Aug 12, 2022

View reviewed changes

zelliott mentioned this pull request Aug 15, 2022

[api-extractor] Add options to include forgotten exports in API report and doc model files #3552

Merged

octogonz reviewed Aug 24, 2022

View reviewed changes

Handle some Members navigation step edge cases

b68fec6

zelliott mentioned this pull request Aug 26, 2022

[api-extractor] Fix #3593 Incorrect canonical reference to aliased class in .api.json #3602

Merged

zelliott requested a review from octogonz August 30, 2022 22:47

octogonz approved these changes Sep 2, 2022

View reviewed changes

Merge branch 'main' into refs

19f91a8

# Conflicts: # build-tests/install-test-workspace/workspace/common/pnpm-lock.yaml

octogonz enabled auto-merge September 2, 2022 07:03

Convert this to a MINOR release; technically it is a bug fix, but the…

7f2b01a

… logic change is nontrivial

octogonz disabled auto-merge September 2, 2022 07:12

octogonz merged commit 4de2adf into microsoft:main Sep 2, 2022

octogonz mentioned this pull request Sep 13, 2022

[api-extractor] 7.30.0 causes tryGetEntityForIdentifier() called for an identifier that was not analyzed #3620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-extractor] Fix incorrect declaration references for symbols not exported from the package's entry point #3584

[api-extractor] Fix incorrect declaration references for symbols not exported from the package's entry point #3584

zelliott commented Aug 12, 2022 •

edited

Loading

zelliott Aug 12, 2022 •

edited

Loading

octogonz Aug 23, 2022

zelliott Aug 25, 2022 •

edited

Loading

zelliott Aug 12, 2022

octogonz Aug 24, 2022

octogonz Aug 24, 2022

octogonz Aug 24, 2022

octogonz Aug 24, 2022

octogonz Aug 24, 2022

zelliott Aug 24, 2022

octogonz Aug 24, 2022

octogonz Aug 24, 2022 •

edited

Loading

zelliott Aug 25, 2022

octogonz commented Sep 2, 2022

		@@ -54,13 +54,7 @@ export class ApiModelGenerator {
		public constructor(collector: Collector) {

[api-extractor] Fix incorrect declaration references for symbols not exported from the package's entry point #3584

[api-extractor] Fix incorrect declaration references for symbols not exported from the package's entry point #3584

Conversation

zelliott commented Aug 12, 2022 • edited Loading

Summary

Details

How it was tested

zelliott Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelliott Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1. Interesting case with inline type destructuring

Choose a reason for hiding this comment

2. Case involving "typeof"

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3. Case involving preact

octogonz Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octogonz commented Sep 2, 2022

zelliott commented Aug 12, 2022 •

edited

Loading

zelliott Aug 12, 2022 •

edited

Loading

zelliott Aug 25, 2022 •

edited

Loading

octogonz Aug 24, 2022 •

edited

Loading