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

fwienber · 2022-08-26T19:41:44Z

Summary

api-extractor currently does not support packages with multiple entry points.
As a workaround, multiple entry points can be aggregated through a single entry point, directly or indirectly re-exporting them.
This works as long as their original names are used. However, when trying to rename exports, for example because they are grouped in directories and when "flattening", name-clashes must be avoided, api-extractor produces document model (*.api.json) files with incorrect canonical references. Both renaming and moving a module into a namespace do not work:

Renaming: export { Foo as subpath_Foo } from "./subpath/Foo"
Namespacing: import * as subpath from "./subpath"; export { subpath } (this is the workaround api-extractor suggests for export * as subpath from "./subpath" not yet being supported)

In both cases, the subpath information is lost in canonical references to Foo.
For details and an example, see #3593.

Fixes #3593

Details

After a pointer by @zelliott, extended Collector to offer a new function to retrieve the "emit name" of a given ts.Symbol. The emitName property can be fetched from a CollectorEntity and is rename-aware. DeclarationReferenceGenerator calls this function to use the correct emit name if the referenced symbol has been renamed via a re-export. This fixes the first part of the bug.

Concerning the second part of the bug, namespaced re-exports:
When DeclarationReferenceGenerator#_getParentReference() looks for a parent symbol, it must also resolve namespace parent symbols the Collector derived from import * as ___. To that end, Collector now has a function getExportingNamespace() that finds namespace imports of the given symbol by looking up the parent namespace in the corresponding CollectorEntity (if present).
This new function is then used by DeclarationReferenceGenerator to compute the correct parent DeclarationReference. Also there, handling of namespaces had to be improved.

How it was tested

Added two test scenarios, one for each part of the bug:
docReferencesAlias uses renaming, docReferencesNamespaceAlias uses namespaces for re-export.
When building the build-tests, there is no git diff, so the expected *.api.json files with the correct canonical references are now generated.

ghost · 2022-08-26T19:41:56Z

All CLA requirements met.

zelliott

Thanks for the draft PR. Left some initial comments. I need to dive into this deeper, hoping to get to that this weekend. Test cases look good. 👍

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

apps/api-extractor/src/collector/Collector.ts

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

zelliott · 2022-09-01T03:16:03Z

Hey, sorry for the delay @fwienber! Hoping to take a look tomorrow 👍

fwienber · 2022-09-01T08:28:25Z

No worries! Looks like we live in time zones that unfortunately have minimal working hours overlap (CEST = UTC+2 here)...

zelliott · 2022-09-02T01:28:47Z

@octogonz JFYI: This PR is will have merge conflicts with #3584 and #3552 and will be a bit easier to implement and review once those two PRs are submitted.

zelliott

I think the nameForEmit part LGTM, left a few comments about the namespace imports piece. LMK if I can help clarify anything!

fwienber · 2022-09-02T19:31:58Z

@octogonz JFYI: This PR is will have merge conflicts with #3584 and #3552 and will be a bit easier to implement and review once those two PRs are submitted.

Great, looks like they are already merged!
Then, I'll force-push this branch with my rebased latest solution!

Maybe newlineKind: 'os' should be the default for api-extractor config (ExtractorConfig.ts) anyway, but for now, let's make it explicit in the test configuration. Another possible scenario is that the tests are run under Windows, but git has been configured to used 'lf'. Then, using 'os' line endings would still lead to a git diff. But detecting the git setting is probably beyond what the test runner should do.

"Incorrect canonical reference to aliased class in .api.json" microsoft#3593 Fix first scenario from microsoft#3593. Added test scenario "docReferencesAlias", inspired by the test repo referenced in the GitHub issue: `Item` and `Options` are default exports of two separate source files, and `index` re-exports both, renaming `Options` to `renamed_Options` (preserving the "namespacing" information of the directory). `DeclarationReferenceGenerator` now receives a function to resolve a Symbol to its emit name. `ApiModelGenerator` fetches this function from `Collector`, which now maintains another map from symbol to `CollectorEntity`. If for a given symbol, there is a `CollectorEntity`, then its `emitName` is returned, the symbol's name otherwise. While this change fixes the `export { __ as __ }` scenario, for _some_ reason, it changes the output of one other test scenario, namely "ambientNameConflict". In its `*.api.json` file, some reference to the class re-exported as `MyPromise`, the name clash of localFile's `Promise` and the global `Promise` now leads to `Promise_2` being used instead of `Promise`, the old expected result that was also wrong, because it should have been `MyPromise`, and `MyPromise` should appear in `*.api.json`, but that is something completely different, so I'll ignore the problem for now and check in the changed expected `*.api.json` of "ambientNameConflict".

"Incorrect canonical reference to aliased class in .api.json" microsoft#3593 This fixes the other aliasing, namely through (nested) namespaces. Added a test scenario "docReferencesNamespaceAlias", which consists of nested namespaces that have their own index.d.ts, re-exporting the default export of each sibling file and their respective sub-namespace (top-level -> 'renamed' -> 'sub'). The *.api.json canonicalReferences now correctly use the nested namespace "fully-qualified name". Again, some other test result changed unexpectedly, maybe revealing an existing bug / wrong expected result. The scenarios are "exportImportStarAs2", "importEquals" and "includeForgottenExports". The pattern is that before, namespace and class were separate tokens, but are now aggregated to one dot-separated expression. I hope that these are improvements, but I am not sure, so this should be reviewed. Implementation: --------------- When DeclarationReferenceGenerator#_getParentReference() looks for a parent symbol, it must also resolve namespace parent symbols the Collector derived from 'import * as ___'. To that end, Collector now has a method getExportingNamespace() that pulls namespace imports from the CollectorEntities of a namespace symbol (if present). For that, namespace imports are now also registered in the _entitiesBySymbol Map. In DeclarationReferenceGenerator, the handling of namespaces had to be improved. _getNavigationToSymbol() now detects namespaces, even behind an alias, and then always returns "." as separator. To find the parent, it also considers namespaces now. _symbolToDeclarationReference() takes care not to follow an alias that leads to a *-imported namespace, because the target is the imported source file that no longer allows to retrieve the namespace name.

fwienber · 2022-09-02T20:29:14Z

@octogonz JFYI: This PR is will have merge conflicts with #3584 and #3552 and will be a bit easier to implement and review once those two PRs are submitted.

Great, looks like they are already merged! Then, I'll force-push this branch with my rebased latest solution!

Done, ready for review!

fwienber · 2022-09-07T13:10:50Z

bump

zelliott · 2022-09-08T00:32:25Z

Hey @fwienber - I opened fwienber#2 for some proposed changes to this PR. LMK what you think - feel free to merge in/modify as you see fit.

I don't actually have merge powers in this repository - I'm just a contributor - we'll need @octogonz for that. We'll also definitely want him to take a look at this PR... AstNamespaceImports are tricky.

zelliott · 2022-09-07T21:46:12Z

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

+    // First case: the symbol is exported via a namespace
+    const exportingNamespace: ts.Symbol | undefined = this._collector.getExportingNamespace(symbol);
+    if (exportingNamespace) {
+      return this._symbolToDeclarationReference(exportingNamespace, exportingNamespace.flags, true);
+    }


What if a symbol is exported from both a namespace and the entry point? Consider the following:

// index.ts import * as i1 from './internal'; import { SomeClass } from './internal'; export { i1, SomeClass } // internal.ts export class SomeClass {}

What should the declaration reference of SomeClass be? Today, API Extractor will actually write two items to the doc model for SomeClass:

my-package!SomeClass:class as a child of the ApiEntryPoint.

my-package!i1.SomeClass:class as a child of the ApiNamespace my-package!i1.

(Note that today API Extractor will actually write only the latter to the doc model, but this is a recent bug from #3552 and relatively trivial to fix. I'm working on it.)

Ideally API Extractor would only write SomeClass once to the doc model... so which declaration reference is right? Today, your PR generates my-package!i1.SomeClass:class for reference tokens, in part due to the logic highlighted by this comment.

Another situation - suppose a symbol is exported from multiple namespaces, in some convoluted way like this:

// index.ts import * as i1 from './intermediate1'; import * as i2 from './intermediate2'; export { i1, i2 } // intermediate1.ts import * as internal from './internal'; export { internal } // intermediate2.ts import * as internal from './internal'; export { internal } // internal.ts export class SomeClass {}

What should the declaration reference of SomeClass be? Again, today, API Extractor writes two items for SomeClass:

my-package!i1.internal.SomeClass

my-package!i2.internal.SomeClass

And today, your PR is indeterministic in which one it generates for reference tokens.

I think fundamentally this is another instance of #1308. I'm not sure what the right behavior is. But I do think we want the behavior to be somewhat deterministic and robust to shuffling of API items. One idea is to choose the shortest ID, and among IDs of the same length, choose the one that comes first alphabetically. That being said, I'm not sure if this open question should block this PR.

@octogonz - what do you think?

Seems like a bad idea in the first place to expose the same module under two different names. However, if you want to support this use case, maybe the developer should have some means to specify which alias is the "canonical" one and thus has precedence. Could be a new api-extractor configuration or even a new TSDoc tag.

One idea is to choose the shortest ID, and among IDs of the same length, choose the one that comes first alphabetically.

This sounds good to me. Maybe we could also provide a TSDoc tag that allows the choice to be explicitly specified, similar to suggestions for the other ae-ambiguous-ids issues.

Some proposed changes to microsoft#3602

fwienber · 2022-09-08T15:17:23Z

@zelliott thanks for the PR, I merged it without changes!
Now, I'm playing with explicit TSDoc references (@link, @inheritDoc).
It seems that when api-extractor tries to resolve such references, there is another code path which still cannot handle AstNamespaceImports:
https://github.com/fwienber/rushstack/blob/fwienber/api-extractor-fix-3593-reference-alias/apps/api-extractor/src/analyzer/AstReferenceResolver.ts#L94-L96
Unfortunately, despite some debugging, I couldn't figure out how rootAstEntity instanceof AstNamespaceImport would have to be handled, i.e. how to find the corresponding AstDeclaration. I always end up with ts.Declaration / ts.Symbol, and these do not seem to be part of the AstSymbolTable (_astDeclarationsByDeclaration).
I hoped maybe you can help with this, too?
Otherwise, it seems there is no syntax currently supported by api-extractor to reference those namespace-re-exported classes in TSDoc, which would be a shame now that they work for references derived from the code.

zelliott · 2022-09-08T15:30:26Z

Sure! I'm taking another look at this PR right now.

Do you mind sharing a code example of the issue you're describing? I can try to help with this as well - but probably worth doing in a follow-up PR as this one is somewhat large as-is.

fwienber · 2022-09-08T15:37:26Z

Do you mind sharing a code example of the issue you're describing?

It was in my "real world" example, but should be easy to add to some scenario. Just a minute!

fwienber · 2022-09-08T15:53:25Z

Okay, just go to build-tests/api-extractor-scenarios/src/docReferencesNamespaceAlias/Item.ts and add {@inheritDoc renamed.Options} to the TSDoc comment of class Item.
Unfortunately, when building the api-extractor-scenarios, all unresolved references messages are swallowed, but setting a breakpoint in the line I posted above, where the error is thrown, and then debug the build script of build-tests/api-extractor-scenarios/package.json should do the job.
Then, rootAstEntity instanceof AstSymbol is false because it is the AstNamespaceImport with the name renamed, so it is exactly what we want, but AstReferenceResolver cannot handle it.

zelliott

I think this looks good to me! Regarding the @link and @inheritDoc stuff - I'll take a look, but I don't see it as blocking this PR.

build-tests/api-extractor-scenarios/etc/exportImportStarAs2/api-extractor-scenarios.api.json

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

@zelliott

As suggested by @zelliott, the logic when to fall back to Navigation.Exports (`.`) must use `consumable`, not `exported`. Updated the improved build-tests/api-extractor-scenarios results.

…extractor-fix-3593-reference-alias # Conflicts: # apps/api-extractor/src/collector/CollectorEntity.ts # build-tests/api-extractor-scenarios/etc/exportImportStarAs2/api-extractor-scenarios.api.json # build-tests/api-extractor-scenarios/etc/namespaceImports/api-extractor-scenarios.api.json # build-tests/api-extractor-scenarios/etc/namespaceImports2/api-extractor-scenarios.api.json

microsoft#3622

octogonz · 2022-09-12T22:51:01Z

@fwienber I've reverted your .gitignore changes and moved them to a separate PR #3622

…ngConsumableParent()" method because it does nontrivial work

apps/api-extractor/src/collector/CollectorEntity.ts

...icrosoft/api-extractor/fwienber-api-extractor-fix-3593-reference-alias_2022-08-29-07-53.json

octogonz · 2022-09-13T00:22:43Z

@fwienber Thanks for contributing this fix! And thanks @zelliott for reviewing the code!

🚀 Released with API Extractor 7.31.0

…r-fix-3593-reference-alias [api-extractor] Fix microsoft#3593 Incorrect canonical reference to aliased class in .api.json

fwienber mentioned this pull request Aug 26, 2022

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

Closed

zelliott reviewed Aug 26, 2022

View reviewed changes

fwienber marked this pull request as ready for review August 29, 2022 11:07

fwienber requested review from iclanton, octogonz, apostolisms and D4N14L as code owners August 29, 2022 11:07

fwienber requested a review from zelliott August 31, 2022 18:25

zelliott suggested changes Sep 2, 2022

View reviewed changes

fwienber and others added 5 commits September 2, 2022 21:50

Diff after building: Update build-test exportImportStarAs2 result

8a1cbd3

Ignore IntelliJ IDEA files

6bc0676

fwienber force-pushed the fwienber/api-extractor-fix-3593-reference-alias branch from 032e401 to ef387a7 Compare September 2, 2022 20:18

fwienber requested a review from zelliott September 2, 2022 20:20

Changes to microsoft#3602

97e4593

zelliott reviewed Sep 8, 2022

View reviewed changes

zelliott mentioned this pull request Sep 8, 2022

Some proposed changes to #3602 fwienber/rushstack#2

Merged

zelliott and others added 2 commits September 7, 2022 17:42

Added a comment

1e11c5c

Merge pull request #2 from zelliott/pr-3602-review

b02f81b

Some proposed changes to microsoft#3602

zelliott mentioned this pull request Sep 8, 2022

[api-extractor] Fix a bug where an item is exported from both the entry point and an exported AstNamespaceImport. #3619

Merged

zelliott approved these changes Sep 8, 2022

View reviewed changes

build-tests/api-extractor-scenarios/etc/exportImportStarAs2/api-extractor-scenarios.api.json Outdated Show resolved Hide resolved

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

fwienber and others added 4 commits September 8, 2022 19:25

[api-extractor] Fix logic when to fall back to Navigation.Exports

844f15b

As suggested by @zelliott, the logic when to fall back to Navigation.Exports (`.`) must use `consumable`, not `exported`. Updated the improved build-tests/api-extractor-scenarios results.

Add change log entry for fix of microsoft#3593

f423294

rush build

a2f2485

octogonz mentioned this pull request Sep 12, 2022

Update the .gitignore template to ignore files generated by IntelliJ IDEA #3622

Merged

Revert the .gitignore change; we'll implement it separately in this PR:

4df3810

microsoft#3622

Convert "firstExportingConsumableParent" property to "getFirstExporti…

6d975b5

…ngConsumableParent()" method because it does nontrivial work

octogonz reviewed Sep 12, 2022

View reviewed changes

apps/api-extractor/src/collector/CollectorEntity.ts Show resolved Hide resolved

octogonz added 2 commits September 12, 2022 16:05

Update change file

6f1811e

Update install-test-workspace lockfile

1ba1066

octogonz reviewed Sep 12, 2022

View reviewed changes

...icrosoft/api-extractor/fwienber-api-extractor-fix-3593-reference-alias_2022-08-29-07-53.json Show resolved Hide resolved

octogonz approved these changes Sep 12, 2022

View reviewed changes

octogonz merged commit fb400b6 into microsoft:main Sep 12, 2022

fwienber deleted the fwienber/api-extractor-fix-3593-reference-alias branch September 13, 2022 07:22

Qjuh pushed a commit to discordjs/rushstack that referenced this pull request Nov 7, 2023

Changes to microsoft#3602

7519f96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fwienber commented Aug 26, 2022 •

edited

Loading

ghost commented Aug 26, 2022 •

edited by ghost

Loading

zelliott left a comment •

edited

Loading

zelliott commented Sep 1, 2022

fwienber commented Sep 1, 2022

zelliott commented Sep 2, 2022

zelliott left a comment

fwienber commented Sep 2, 2022 •

edited

Loading

fwienber commented Sep 2, 2022

fwienber commented Sep 7, 2022

zelliott commented Sep 8, 2022

zelliott Sep 7, 2022

fwienber Sep 8, 2022

octogonz Sep 13, 2022

fwienber commented Sep 8, 2022

zelliott commented Sep 8, 2022

fwienber commented Sep 8, 2022

fwienber commented Sep 8, 2022 •

edited

Loading

zelliott left a comment

octogonz commented Sep 12, 2022

octogonz commented Sep 13, 2022

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

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

Conversation

fwienber commented Aug 26, 2022 • edited Loading

Summary

Details

How it was tested

ghost commented Aug 26, 2022 • edited by ghost Loading

zelliott left a comment • edited Loading

Choose a reason for hiding this comment

zelliott commented Sep 1, 2022

fwienber commented Sep 1, 2022

zelliott commented Sep 2, 2022

zelliott left a comment

Choose a reason for hiding this comment

fwienber commented Sep 2, 2022 • edited Loading

fwienber commented Sep 2, 2022

fwienber commented Sep 7, 2022

zelliott commented Sep 8, 2022

zelliott Sep 7, 2022

Choose a reason for hiding this comment

fwienber Sep 8, 2022

Choose a reason for hiding this comment

octogonz Sep 13, 2022

Choose a reason for hiding this comment

fwienber commented Sep 8, 2022

zelliott commented Sep 8, 2022

fwienber commented Sep 8, 2022

fwienber commented Sep 8, 2022 • edited Loading

zelliott left a comment

Choose a reason for hiding this comment

octogonz commented Sep 12, 2022

octogonz commented Sep 13, 2022

fwienber commented Aug 26, 2022 •

edited

Loading

ghost commented Aug 26, 2022 •

edited by ghost

Loading

zelliott left a comment •

edited

Loading

fwienber commented Sep 2, 2022 •

edited

Loading

fwienber commented Sep 8, 2022 •

edited

Loading