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] Improve SymbolAnalyzer to support "export * from" #949

Closed
octogonz opened this issue Nov 17, 2018 · 7 comments
Closed

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

octogonz opened this issue Nov 17, 2018 · 7 comments
Labels
enhancement The issue is asking for a new feature or design change

Comments

@octogonz
Copy link
Collaborator

The SymbolAnalyzer currently relies on TypeScriptHelpers.getImmediateAliasedSymbol() for determining whether a symbol was imported from an external package, and if so what module path was used to import it.

This approach cannot handle export * from '____' constructs, because there is no symbol alias that can be used to discover the export line. In API Extractor 6 this problem mostly affects the .d.ts rollups. For the API report, it simply includes extra definitions that technically belong to an external package.

For API Extractor 7 this problem is more serious, because everything uses the .d.ts rollups engine.

I have a plan to improve the SymbolAnalyzer, but it needs to wait until the initial AE7 branch is merged.

@octogonz octogonz added enhancement The issue is asking for a new feature or design change blocked We can't proceed because we're waiting for something labels Nov 17, 2018
@octogonz
Copy link
Collaborator Author

When the .d.ts rollups encounter this issue, it sometimes produces this error:

"Program Bug: The symbol ____ is being imported after it was already registered as non-imported"

The reason is that it incorrectly believes the symbol is part of the current project, and then later discovers the inconsistency.

octogonz pushed a commit that referenced this issue Nov 17, 2018
"The symbol X was also exported as Y; this is not supported yet" (GitHub #950)

"Program Bug: The symbol GulpTask is being imported after it was already registered as non-imported" (GitHub #949)
octogonz pushed a commit that referenced this issue Nov 20, 2018
Delete deadwood files

Rename src -> src-old

Bringing over files

Bring over more files

Bring over more files

Bring over more files

Bring over more files

The dtsRollup generator now builds and works correctly

Initial skeleton of AstModel concept

Initial prototype of mixins approach

Fix serialization of AstMemberMixin

Add canonical selector and StaticMixin

Replace canonical selector with canonical reference, and support member overloading

Add ApiInterface and PropertySignature with declaration merging for interfaces

Rename "parameters" to "options" to avoid confusion with TypeScript function parameters

Implement parameter serialization

Deserialization is now working

Set up ApiDeclaration base class for tracking signatures

Rename StaticMixin.ts -> ApiStaticMixin.ts

Add signature

ApiParameter is now a proper ApiDeclaration

Convert ApiDeclaration to a mixin

upgrade tsdoc

rush update

Bring over AedocDefinitions.ts

Doc comments are now parsed and serialized into .api.json

ApiParameter returns docs using tryGetBlockByName

Upgrade tsdoc

rush update

Write a header at the top of the .api.json file

- Remove debug code
- Wire up the .api.json writer feature
- Remove the externalJsonFileFolders feature; if someone really needs this, we'll redesign it

Remove obsolete code from TypeScriptHelpers.ts

Update IndentedWriter to use StringBuilder

Add TSDocFlavor field

The `@packageDocumentation` comment is now being emitted again in the .d.ts rollups, sharing the same scanner as ModelBuilder

Bring over Markup.ts and MarkupElement.ts

Add tsdoc dependency for api-documenter

Update MarkdownRenderer.ts to build again

Expose ModelBuilder as public API

Temporary workaround for declarations that get emitted weirdly by the compiler

Add custom DocNode subclasses that will eliminate the need for MarkupElement

Make ApiModel and ApiPackage easier to traverse

Add ApiItem.getHierarchy()

Add ApiReleaseTagMixin

Add a test that repros the bug where EcmaScript symbols aren't rolled up properly

Fix the bug where EcmaScript symbols were not rolled up correctly

rush update

Add jest as a dependency so we can debug tests

MarkdownDocumenter.ts is now more or less working using TSDoc instead of Markup

Fix various files to use the updated TSDoc API

Add isBaseClassOf() helper for mixins

Export ReleaseTag

Add ApiMethodSignature and ApiProperty classes

Add ApiResultTypeMixin

Add ApiPropertyItem

Fix ApiProperty.kind

Set up new skeleton for MarkdownDocumenter.ts

Function parameters and return value are now documented

Generalize MarkdownRenderer to support link resolution

Implement a basic TSDoc declaration reference resolver

Rename MarkdownRenderer.ts -- MarkdownEmitter.ts

Move the test files

Rename MarkdownRenderer --> MarkdownEmitter

Convert MarkdownEmitter into a non-static class, and separate SimpleWriter into its own source file

Separate the non-TSDoc handling into a CustomMarkdownEmitter subclass

If the link text is omitted, automatically generate it based on the ApiItem name

Add ApiDeclarationMixin.getSignatureWithModifiers()

YamlDocumenter and OfficeYamlDocumenter finally build again

Move documenter classes under "documenters" folder

Move documenter classes under "documenters" folder

Minor fixes

Moving file

Updating imports

Move the core DtsRollupGenerator analysis down into ExtractorContext

Rename ModelBuilder to ApiModelGenerator

Rename ModelBuilder to ApiModelGenerator

Rename ExtractorContext --> Collector, and DtsEntry --> CollectorEntity

Rename ExtractorContext --> Collector, and DtsEntry --> CollectorEntity

Separated package state into CollectorPackage, refactored ApiModelGenerator to reuse the analysis from Collector

Bring over ReviewFileGenerator.ts

Release tag calculation is now centralized in Collector

Fix for GitHub issue #936 where doc comments were not being read for variable declarations

Fix some miscellaneous issues for YamlDocumenter.ts

Complete _writeAedocSynopsis()

Fix .d.ts rollup emitting of doc comments for packages and variable declarations

The API Review File now sorts nested members and injects _getAedocSynopsis() comments while preserving .d.ts indentation

Don't show redundant release tags in the API review file

Fix an issue where getSortKeyIgnoringUnderscore() was case-sensitive and

Fix an issue where DeclarationMetadata.needsDocumentation was false when the doc comment was completely missing

The test *.d.ts files are now built correctly

Disabled API Extractor for projects with incompatibilities:

"The symbol X was also exported as Y; this is not supported yet" (GitHub #950)

"Program Bug: The symbol GulpTask is being imported after it was already registered as non-imported" (GitHub #949)

Add `"tsdocFlavor": "AEDoc"` to projects that build with API Extractor

Add missing release tags for declarations that are now picked up by AE7

Don't report release tag errors for the default export

Fix table of contents generation

The MarkdownDocumenter and YamlDocumenter test output is now correct

Improve appearance of VariableDeclaration in .api.ts files

Delete left over files from src-old/

Add a test illustrating usage of the `@example` tag

MarkdownDocumenter now supports the `@example` tag from TSDoc

Move IndentedWriter.ts under src/api

Bring over IStringBuilder from TSDoc

Merge SimpleWriter and IndentedWriter into a single class

MarkdownDocumenter now writes `@deprecated` warnings
octogonz pushed a commit that referenced this issue Nov 22, 2018
Delete deadwood files

Rename src -> src-old

Bringing over files

Bring over more files

Bring over more files

Bring over more files

Bring over more files

The dtsRollup generator now builds and works correctly

Initial skeleton of AstModel concept

Initial prototype of mixins approach

Fix serialization of AstMemberMixin

Add canonical selector and StaticMixin

Replace canonical selector with canonical reference, and support member overloading

Add ApiInterface and PropertySignature with declaration merging for interfaces

Rename "parameters" to "options" to avoid confusion with TypeScript function parameters

Implement parameter serialization

Deserialization is now working

Set up ApiDeclaration base class for tracking signatures

Rename StaticMixin.ts -> ApiStaticMixin.ts

Add signature

ApiParameter is now a proper ApiDeclaration

Convert ApiDeclaration to a mixin

upgrade tsdoc

rush update

Bring over AedocDefinitions.ts

Doc comments are now parsed and serialized into .api.json

ApiParameter returns docs using tryGetBlockByName

Upgrade tsdoc

rush update

Write a header at the top of the .api.json file

- Remove debug code
- Wire up the .api.json writer feature
- Remove the externalJsonFileFolders feature; if someone really needs this, we'll redesign it

Remove obsolete code from TypeScriptHelpers.ts

Update IndentedWriter to use StringBuilder

Add TSDocFlavor field

The `@packageDocumentation` comment is now being emitted again in the .d.ts rollups, sharing the same scanner as ModelBuilder

Bring over Markup.ts and MarkupElement.ts

Add tsdoc dependency for api-documenter

Update MarkdownRenderer.ts to build again

Expose ModelBuilder as public API

Temporary workaround for declarations that get emitted weirdly by the compiler

Add custom DocNode subclasses that will eliminate the need for MarkupElement

Make ApiModel and ApiPackage easier to traverse

Add ApiItem.getHierarchy()

Add ApiReleaseTagMixin

Add a test that repros the bug where EcmaScript symbols aren't rolled up properly

Fix the bug where EcmaScript symbols were not rolled up correctly

rush update

Add jest as a dependency so we can debug tests

MarkdownDocumenter.ts is now more or less working using TSDoc instead of Markup

Fix various files to use the updated TSDoc API

Add isBaseClassOf() helper for mixins

Export ReleaseTag

Add ApiMethodSignature and ApiProperty classes

Add ApiResultTypeMixin

Add ApiPropertyItem

Fix ApiProperty.kind

Set up new skeleton for MarkdownDocumenter.ts

Function parameters and return value are now documented

Generalize MarkdownRenderer to support link resolution

Implement a basic TSDoc declaration reference resolver

Rename MarkdownRenderer.ts -- MarkdownEmitter.ts

Move the test files

Rename MarkdownRenderer --> MarkdownEmitter

Convert MarkdownEmitter into a non-static class, and separate SimpleWriter into its own source file

Separate the non-TSDoc handling into a CustomMarkdownEmitter subclass

If the link text is omitted, automatically generate it based on the ApiItem name

Add ApiDeclarationMixin.getSignatureWithModifiers()

YamlDocumenter and OfficeYamlDocumenter finally build again

Move documenter classes under "documenters" folder

Move documenter classes under "documenters" folder

Minor fixes

Moving file

Updating imports

Move the core DtsRollupGenerator analysis down into ExtractorContext

Rename ModelBuilder to ApiModelGenerator

Rename ModelBuilder to ApiModelGenerator

Rename ExtractorContext --> Collector, and DtsEntry --> CollectorEntity

Rename ExtractorContext --> Collector, and DtsEntry --> CollectorEntity

Separated package state into CollectorPackage, refactored ApiModelGenerator to reuse the analysis from Collector

Bring over ReviewFileGenerator.ts

Release tag calculation is now centralized in Collector

Fix for GitHub issue #936 where doc comments were not being read for variable declarations

Fix some miscellaneous issues for YamlDocumenter.ts

Complete _writeAedocSynopsis()

Fix .d.ts rollup emitting of doc comments for packages and variable declarations

The API Review File now sorts nested members and injects _getAedocSynopsis() comments while preserving .d.ts indentation

Don't show redundant release tags in the API review file

Fix an issue where getSortKeyIgnoringUnderscore() was case-sensitive and

Fix an issue where DeclarationMetadata.needsDocumentation was false when the doc comment was completely missing

The test *.d.ts files are now built correctly

Disabled API Extractor for projects with incompatibilities:

"The symbol X was also exported as Y; this is not supported yet" (GitHub #950)

"Program Bug: The symbol GulpTask is being imported after it was already registered as non-imported" (GitHub #949)

Add `"tsdocFlavor": "AEDoc"` to projects that build with API Extractor

Add missing release tags for declarations that are now picked up by AE7

Don't report release tag errors for the default export

Fix table of contents generation

The MarkdownDocumenter and YamlDocumenter test output is now correct

Improve appearance of VariableDeclaration in .api.ts files

Delete left over files from src-old/

Add a test illustrating usage of the `@example` tag

MarkdownDocumenter now supports the `@example` tag from TSDoc

Move IndentedWriter.ts under src/api

Bring over IStringBuilder from TSDoc

Merge SimpleWriter and IndentedWriter into a single class

MarkdownDocumenter now writes `@deprecated` warnings

rush update

Manually reapply the fix from PR #948 which got lost in the merge

Improve how Sort.compareByValue() handles "null" and "undefined", and add a unit test

Add test cases that illustrate Enums being handled incorrectly

Update .api.ts files

Fix the issue where Enums were sorted incorrectly by Span.

Add a test case for access modifiers

Add AstDeclaration.modifierFlags and trim private members from *.api.ts and *.api.json
@octogonz octogonz changed the title [api-extractor] Improve SymbolAnalyzer algorithm [api-extractor] Improve SymbolAnalyzer to support "export * from" Nov 22, 2018
@octogonz
Copy link
Collaborator Author

I encountered this imported after it was already registered as non-imported error for a definition like this:

src/index.ts

export { SomeApi } from 'another-package';

The error only occurs if SomeApi was reachable by some other type alias. Otherwise it simply gets treated as a local reference. We should include this test case when validating the fix.

@nvh
Copy link

nvh commented Nov 30, 2018

I encountered this error importing the EditorState from draft-js and using it in a class
A minimal reproduction looks like this:

import { EditorState } from "draft-js"
export class Text {
    editorState: EditorState
}

making the editorState private fixes the problem.
Example project:
registerd-as-non-imported.zip
Reproduction steps:
screenshot 2018-11-30 at 16 05 03

@octogonz
Copy link
Collaborator Author

Thanks for reporting this! I'll make sure we test your case as part of this fix.

@octogonz
Copy link
Collaborator Author

octogonz commented Dec 20, 2018

I encountered this error importing the EditorState from draft-js and using it in a class
A minimal reproduction looks like this:

import { EditorState } from "draft-js"
export class Text {
    editorState: EditorState
}

This is actually an instance of the other issue #663 which involves the export { x } construct.

Update: On further investigation this turned out to be an unrelated problem. I have opened Issue #1001 to track it

@octogonz octogonz removed the blocked We can't proceed because we're waiting for something label Dec 20, 2018
@octogonz
Copy link
Collaborator Author

Support for the export * from '____' construct is implemented in my upcoming PR #1002

@octogonz
Copy link
Collaborator Author

Fixed in PR #1002 except for the special case being tracked by issue #1029

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is asking for a new feature or design change
Projects
None yet
Development

No branches or pull requests

2 participants