[api-extractor] Improve symbol analyzer to support "export * from" constructs #1002

octogonz · 2018-12-20T09:43:08Z

This PR turned into a full rewrite of the symbol analyzer, adding the following capabilities:

Support export * from './localFile'; by merging those declaration into the entry point as before
Support export * from 'external-package' by emitting an export * in the output
Support export { A } from './localFile'; where ./localFile.ts obtains A by doing export * from 'external-package';
Allow a declaration to be exported more than once:

class X { }
export { X as A }
export { X as B}

octogonz · 2018-12-20T09:48:30Z

@natalieethell FYI #Resolved

adventure-yunfei · 2018-12-24T16:05:12Z

FYI: The review file generating will break with export from external package like following:

export { SemVer } from 'semver';

The error is: Error: Child declaration not found for the specified node.

Note that the test case api-extractor-test-02 also includes a similar declaration export { ReexportedClass as RenamedReexportedClass3 } from 'api-extractor-test-01';, but it succeeds, because the nominal is set as true as the check this._packageMetadataManager.isAedocSupportedFor passes and so other declarations are parsed.

And, in such case that this._packageMetadataManager.isAedocSupportedFor returns true, we'll get the similar issue that introduced by namespace (see related issue #1001). The real cause in such case is: if we import something from another package, but it's not the rootAstSymbol, it'll always fail since the later check (followAliasesResult.astImport && !astSymbol.imported) will always be true (the astSymbol itself has "astImport", but its rootAstSymbol doesn't).

I found this problem when trying to add support for export * as blah from './local-module';. Seems this related problem is really tough, cause of some special case handling... #Resolved

adventure-yunfei · 2018-12-24T16:22:11Z

And another one, also for the case:

export { SemVer } from 'semver';

The generated api.json would include detail type declarations for Semver (because we analyzed it as the check this._program.isSourceFileFromExternalLibrary returned false). But since we won't analyze it now, the new api.json will contain nothing except the name Semver... It was a wrong condition checking but seemed to produce the right results~~ #Resolved

octogonz · 2018-12-24T20:04:36Z

@adventure-yunfei I just pushed an update I was working on that hopefully should address this case.

BTW the main idea behind Error: Child declaration not found for the specified node is that nodes from an external package are treated as "nominal" (i.e. only partially analyzed) because we don't expect for any of the generators to process them. Then when that turns out not to be the case, the expected associated AstDeclaration is not found. These bugs should be eliminated once we have fixed all the cases where the symbol analyzer mistakenly treats external symbols as being part of the local project.

The algorithm in my PR branch is still missing a major case. It should now correctly handle these cases:

src/index.ts

export * from './SomeFile';
export * from 'other-package';

...if we have src/SomeFile.ts:

export class X { }

But it does not yet handle this case:

src/index.ts

export { X } from './OtherFile';

...if we have src/OtherFile.ts:

export * from 'other-package';

I don't believe there is an easy solution for that. Replacing ts.TypeChecker.getExportsOfModule() was not too difficult, because export * is a simple construct. But as far as I can tell, this second case requires replacing ts.getImmediateAliasedSymbol() which has to handle all the different import/export syntaxes. The underlying problem is that the compiler treats NPM module resolution as a simplistic mechanism for finding source files, and does not actually track NPM package boundaries in any way that can be discovered later by API Extractor.

I'm hoping to get some time over the holiday to finish this. Originally I was going to defer this work until after version 7 was released, but several teams at work are encountering these issues, so they can't use version 7 without this fix. #Resolved

adventure-yunfei · 2018-12-25T02:45:12Z

I've discovered some designs of api-extractor. To solving this kind of "export from external package" problem, my idea is to always analyze them no matter local or external, instead of blocking analyzing for external. api-extractor is treating external symbols too specially and I wonder it may lead to potential risks.

For *.d.ts rollup or review file generator, detail external declarations may not be helpful (as they simply output the original export ... from '...' statements). But for api.json generator, I think it may be.

Consider the case (for api.json generator):

export { ExternalModule } from 'external-package';

For api.json (or related api-documenter output), what it should be? Two choices:

Hide declaration details and just display an reexport statement from the external package
Hide reexport details and display the actual declaration of ExternalModule similar to any other local declaration.

From the api sight, I would prefer the second one.

And, for any other generators including those that may be added in future, I would prefer to give all details (declaration details, reexport informations) to the generator and let themselves to choose what the want to generate. e.g.:

for every local symbol, it would contain declaration details (keep the same)
for every external symbol (no matter it's created directly by an export statement or indirectly referenced), it would contain both declaration details and the external package info (e.g. what's the external package? Is this symbol exported?)

When the *.d.ts and review file generators see an external symbol, they generate a reexport statement; When the api.json generator (or any other generator taking case of declaration details instead of reexport info), it generate the declaration details.

Just my simple thoughts. I'm learning api-extractor ~ #Resolved

octogonz · 2019-01-06T21:52:41Z

@adventure-yunfei wrote:

For api.json (or related api-documenter output), what it should be? Two choices:

Hide declaration details and just display an reexport statement from the external package

Hide reexport details and display the actual declaration of ExternalModule similar to any other local declaration.

From the api sight, I would prefer the second one.

I'm not sure I agree with this. We are encountering this situation in real world scenarios, e.g. office-ui-fabric-react has a number of instances as part of a transitional step to split up a monolithic NPM package into smaller packages.

In my view, this sort of thing:

export { ExternalModule } from 'external-package';

...is its own kind of API item that is similar to a TypeScript type alias. The API reference web site will already have a full set documentation for ExternalModule and all of its members in the section for external-package. It would seem counterproductive to duplicate all of this content in the section for this-package. Besides being confusing for the human audience, and it might negatively impact other systems such as search engine optimization (SEO).

For the *.d.ts rollup, it would be technically incorrect to duplicate the declarations. (For example, the TypeScript compiler will not consider class types to be interchangeable if they contain any private members.)

The *.api.ts review file is a somewhat special scenario. Since this is used to detect compatibility breaks, it may be useful to inline the full signature of the external declaration. Issue #896 is essentially asking for this. In the comments for that issue I gave some reasons why it should perhaps be an "opt-in" behavior.

In any case, it seems clear that API Extractor's analyzer needs to know which package a declaration comes from, in order to solve these sorts of problems. #Resolved

octogonz · 2019-01-06T21:55:21Z

the main idea behind Error: Child declaration not found for the specified node is that nodes from an external package are treated as "nominal" (i.e. only partially analyzed) because we don't expect for any of the generators to process them.

Hmm... Here and here I came across some usages of the term "nominal" to mean "type compatibility based on the name of a type, not just its signature". I wonder if API Extractor should choose a different name for our concept. @nickpape-msft @iclanton #Resolved

[api-extractor] Add unit tests for PR #1002

… * from" constructs

… declarations (i.e. where both CollectorEntity.exported=true and AstSymbol.imported=true)

…ted multiple times (fixes GitHub issue #950)

adventure-yunfei · 2019-01-14T13:21:36Z

In my view, this sort of thing:
export { ExternalModule } from 'external-package';
...is its own kind of API item that is similar to a TypeScript type alias. The API reference web site will already have a full set documentation for ExternalModule and all of its members in the section for external-package. It would seem counterproductive to duplicate all of this content in the section for this-package. Besides being confusing for the human audience, and it might negatively impact other systems such as search engine optimization (SEO).

Agree with you now. Reexported items from external packages should be handled in user side, not the api-extractor core. It's a special case to merge some packages' contents.

#Resolved

… bugs

iclanton · 2019-01-16T01:37:56Z

apps/api-extractor/src/analyzer/TypeScriptHelpers.ts

+
+    // tslint:disable-next-line:no-any
+    return (ts as any).getResolvedModule(sourceFile, moduleNameText);
+  }


Can you split the compiler internals into a separate file so it's clear that's what they are. It'll also be easier to find problems if the compiler internals change. #Resolved

Fixed

In reply to: 248125894 [](ancestors = 248125894)

iclanton · 2019-01-16T01:38:54Z

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

-        if (this._shouldIncludeReleaseTag(releaseTag, dtsKind)) {
+        if (!this._shouldIncludeReleaseTag(releaseTag, dtsKind)) {
+          indentedWriter.writeLine();
+          indentedWriter.writeLine(`/* Excluded from this release type: ${entity.nameForEmit} */`);


nameForEmit [](start = 81, length = 11)

Sanetize? #WontFix

iclanton · 2019-01-16T01:45:45Z

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

+   * This is true if exportNames contains only one string, and the declaration can be exported using the inline syntax
+   * such as "export class X { }" instead of "export { X }".
+   */
+  public get emitWithExportKeyword(): boolean {


emitWithExportKeyword [](start = 13, length = 21)

Maybe shouldInlineExport #Resolved

Good suggestion

In reply to: 248127101 [](ancestors = 248127101)

iclanton

octogonz · 2019-01-16T22:42:50Z

This is fixed now.

In reply to: 449750119 [](ancestors = 449750119)

octogonz · 2019-01-16T23:32:22Z

I renamed it to "nominalAnalysis"

In reply to: 451778545 [](ancestors = 451778545)

… types

octogonz · 2019-01-17T00:52:51Z

This PR was published as the beta release api-extractor@7.0.10 and api-documenter@7.0.15.

To try it, do npm install @microsoft/api-extractor@beta

This was referenced Dec 20, 2018

[api-extractor] InternalError when analyzing EditorState from draft-js #1001

Closed

[api-extractor] Improve SymbolAnalyzer to support "export * from" #949

Closed

octogonz changed the title ~~[api-extractor] Extend symbol analyzer to support "export * from" constructs~~ [api-extractor] Improve symbol analyzer to support "export * from" constructs Dec 20, 2018

octogonz mentioned this pull request Jan 11, 2019

[api-extractor] Add unit tests for PR #1002 #1022

Merged

octogonz pushed a commit that referenced this pull request Jan 13, 2019

Merge pull request #1022 from Microsoft/pgonzal/ae-export-tests

6e7a07b

[api-extractor] Add unit tests for PR #1002

pgonzal added 4 commits January 13, 2019 12:34

Initial prototype of improving the symbol analyzer to support "export…

bced506

… * from" constructs

Ensure isExternalModuleNameRelative() is called consistently

adddd30

Fix up ApiModelGenerator and ReviewFileGenerator to handle reexported…

75d2a90

… declarations (i.e. where both CollectorEntity.exported=true and AstSymbol.imported=true)

Update tests after rebase

d0f872e

octogonz mentioned this pull request Jan 14, 2019

[api-extractor] Fix error: The symbol "X" was also exported as "Y"; this is not supported yet #950

Closed

Generalize CollectorEntity to support a given declaration being expor…

432d977

…ted multiple times (fixes GitHub issue #950)

octogonz force-pushed the pgonzal/ae-export-star branch from 5b870cc to 432d977 Compare January 14, 2019 05:21

pgonzal added 5 commits January 14, 2019 17:58

Improve exportStar3 test cases

95a7ae1

Redesign the fundamental symbol following algorithm to solve exportStar3

eb7a2cc

Move the symbol following code into a separate class ExportAnalyzer.ts

0a472c3

Generate all config files even if some of the tests failed

e04e434

Reintroduce analysis of ImportSpecifier and ImportClause and fix some…

29be5e0

… bugs

octogonz force-pushed the pgonzal/ae-export-star branch from b83f09b to 29be5e0 Compare January 16, 2019 00:11

Eliminate the old SymbolAnalyzer class

ff512cb

iclanton reviewed Jan 16, 2019

View reviewed changes

iclanton approved these changes Jan 16, 2019

View reviewed changes

pgonzal added 3 commits January 15, 2019 18:31

Emit "export * from" in .d.ts rollup and .api.ts

e8842dd

Reintroduce the declarationName logic

b76720a

Implement AstSymboltable.fetchReferencedAstSymbol

2a00e8e

octogonz mentioned this pull request Jan 16, 2019

[api-extractor] Support "import * as __ from __" for local paths #1029

Closed

Reintroduce limited support for ts.SyntaxKind.ExportSpecifier

3f36ff9

pgonzal added 10 commits January 16, 2019 16:08

Clean up logic

19f0204

Rename "nominal" to "nominalAnalysis" to avoid confusion with nominal…

1118a37

… types

Rename emitWithExportKeyword to shouldInlineExport

9c39f61

Split compiler internals into a separate file

1afcdbd

Improve documentation for new code

2757b66

Fix typo

5cbb407

rush change

5bb4088

rush change

9365de2

Normalize newlines in .api.ts files

0904681

rush change

f2e0fd8

octogonz force-pushed the pgonzal/ae-export-star branch from fae4865 to f2e0fd8 Compare January 17, 2019 00:15

pgonzal added 2 commits January 16, 2019 16:16

Merge branch 'master' into pgonzal/ae-export-star

7af89ee

Normalize newlines in .api.ts files

70d4f0b

octogonz merged commit 5022785 into master Jan 17, 2019

octogonz deleted the pgonzal/ae-export-star branch January 17, 2019 00:32

octogonz mentioned this pull request Jan 17, 2019

[api-extractor] Support "export { x }" for converting a source file's exports into a named module #663

Closed

hiranya911 mentioned this pull request May 5, 2021

[api-documenter] Documentation for re-exports from external packages #2672

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-extractor] Improve symbol analyzer to support "export * from" constructs #1002

[api-extractor] Improve symbol analyzer to support "export * from" constructs #1002

octogonz commented Dec 20, 2018 •

edited

Loading

octogonz commented Dec 20, 2018 •

edited

Loading

adventure-yunfei commented Dec 24, 2018 •

edited by octogonz

Loading

adventure-yunfei commented Dec 24, 2018 •

edited by octogonz

Loading

octogonz commented Dec 24, 2018 •

edited

Loading

adventure-yunfei commented Dec 25, 2018 •

edited by octogonz

Loading

octogonz commented Jan 6, 2019 •

edited

Loading

octogonz commented Jan 6, 2019 •

edited

Loading

adventure-yunfei commented Jan 14, 2019 •

edited by octogonz

Loading

iclanton Jan 16, 2019 •

edited by octogonz

Loading

octogonz Jan 16, 2019

iclanton Jan 16, 2019 •

edited by octogonz

Loading

iclanton Jan 16, 2019 •

edited by octogonz

Loading

octogonz Jan 16, 2019

iclanton left a comment

octogonz commented Jan 16, 2019

octogonz commented Jan 16, 2019

octogonz commented Jan 17, 2019 •

edited

Loading

[api-extractor] Improve symbol analyzer to support "export * from" constructs #1002

[api-extractor] Improve symbol analyzer to support "export * from" constructs #1002

Conversation

octogonz commented Dec 20, 2018 • edited Loading

octogonz commented Dec 20, 2018 • edited Loading

adventure-yunfei commented Dec 24, 2018 • edited by octogonz Loading

adventure-yunfei commented Dec 24, 2018 • edited by octogonz Loading

octogonz commented Dec 24, 2018 • edited Loading

adventure-yunfei commented Dec 25, 2018 • edited by octogonz Loading

octogonz commented Jan 6, 2019 • edited Loading

octogonz commented Jan 6, 2019 • edited Loading

adventure-yunfei commented Jan 14, 2019 • edited by octogonz Loading

iclanton Jan 16, 2019 • edited by octogonz Loading

Choose a reason for hiding this comment

octogonz Jan 16, 2019

Choose a reason for hiding this comment

iclanton Jan 16, 2019 • edited by octogonz Loading

Choose a reason for hiding this comment

iclanton Jan 16, 2019 • edited by octogonz Loading

Choose a reason for hiding this comment

octogonz Jan 16, 2019

Choose a reason for hiding this comment

iclanton left a comment

Choose a reason for hiding this comment

octogonz commented Jan 16, 2019

octogonz commented Jan 16, 2019

octogonz commented Jan 17, 2019 • edited Loading

octogonz commented Dec 20, 2018 •

edited

Loading

octogonz commented Dec 20, 2018 •

edited

Loading

adventure-yunfei commented Dec 24, 2018 •

edited by octogonz

Loading

adventure-yunfei commented Dec 24, 2018 •

edited by octogonz

Loading

octogonz commented Dec 24, 2018 •

edited

Loading

adventure-yunfei commented Dec 25, 2018 •

edited by octogonz

Loading

octogonz commented Jan 6, 2019 •

edited

Loading

octogonz commented Jan 6, 2019 •

edited

Loading

adventure-yunfei commented Jan 14, 2019 •

edited by octogonz

Loading

iclanton Jan 16, 2019 •

edited by octogonz

Loading

iclanton Jan 16, 2019 •

edited by octogonz

Loading

iclanton Jan 16, 2019 •

edited by octogonz

Loading

octogonz commented Jan 17, 2019 •

edited

Loading