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] Support "import * as __ from __" for local paths #1029

Open
octogonz opened this issue Jan 16, 2019 · 14 comments

Comments

@octogonz
Copy link
Collaborator

@octogonz octogonz commented Jan 16, 2019

PR #1002 adds support for declarations like this:

import * as React from 'react';

This is a trivial implementation, where we pretend that * is an actual name exported by the "react" package. It does not work for the general form that introduces a new namespace, for example:

// For example, "Button" in controls.ts becomes "controls.Button"
import * as controls from './controls';

This form is called ts.SyntaxKind.NamespaceImport because it is essentially a more general form of the namespace controls { } declaration kind.

As such, it should probably produce an ApiItemKind.Namespace item.

@octogonz

This comment has been minimized.

Copy link
Collaborator Author

@octogonz octogonz commented Jan 17, 2019

@gf3 shared a branch gf3/api-extractor-issue@30de59d that repros this issue.

@octogonz

This comment has been minimized.

Copy link
Collaborator Author

@octogonz octogonz commented Jan 17, 2019

Some other examples were given in the comments for issue #663

@shlomiassaf

This comment has been minimized.

Copy link

@shlomiassaf shlomiassaf commented Jan 21, 2019

@pgonzal I think that those imports should be handled differently, from a usage perspective and rather then semantic...

import * as React from 'react';
import * as controls from './controls';

export class ABC {
  p1: React.Component;
  p2: controls.Control;
}

The code above is similar to this:

import { Component }  from 'react';
import { Control } from './controls';

export class ABC {
  p1: Component;
  p2: Control;
}

The local import is purely semantic...
When generating docs it doesn't make sense to render p2: controls.Control; so the library should find the direct reference and ignore the namespace.

When importing from third party, it's a different story and the way it is rendered is up to the user, just need to make sure there's enough info...

@octogonz

This comment has been minimized.

Copy link
Collaborator Author

@octogonz octogonz commented Jan 27, 2019

@shlomiassaf

FYI API Extractor wants all reachable types to have exported names, mainly for the software engineering reason that it's more difficult to use an API with anonymous types, and also more difficult for the documentation to talk about it.

For your second example, API Extractor should warn about Component, something like this:

import { Component }  from 'react';
import { Control } from './controls';

export class ABC {
  // WARNING: the declaration "Component" is not exported
  p1: Component;
  p2: Control;
}

Whereas in your first example, it should issue this sort of warning:

import * as React from 'react';
import * as controls from './controls';

export class ABC {
  // (no warning because the "React" module is an external package)
  p1: React.Component;

  // WARNING: the declaration "controls" is not exported
  p2: controls.Control;
}

If you mixed them, then the desired behavior is less clear:

import * as React from 'react';
import * as controls from './controls';
export { Control } from './controls';

export class ABC {
  // (no warning because the "React" module is an external package)
  p1: React.Component;

  // WARNING: the declaration "controls" is not exported
  p2: controls.Control;
}

I suppose the ideal behavior might be to realize that controls.Control is a synonym for Control and normalize it to eliminate the controls. part. But this is technically difficult. I haven't thought about it in depth because this problem seems to never occur in any of our code bases heheh.

@shlomiassaf

This comment has been minimized.

Copy link

@shlomiassaf shlomiassaf commented Jan 31, 2019

@octogonz Thanks for clear explanation.

I think that the underlaying issue is more about the concept and direction of this project.
This tool is used for several tasks, document generation is one of them.

We can agree that generating documentation from the json output can happen is a lot of ways, so many that I can't count :)

At it's current state, the project does provide a way to change the behaviour, configuring it, etc...

Taking a lot of assumptions is risky because it means that it will only do and support things that occur in your code bases and that's not a very good approach for a public project.

I see great importance in tsdoc and this tool and I think that wide adoption will help the community because there are a lot of tools for this job, each with pros/cons and having a unified tool from MS is probably the best solution.

But, for that we need flexibility, understanding that your code base might not be the same as another's code base.

For example, let's say I built an angular library that uses a lot of symbols from various @angular libraries and also symbols from rxjs

Now of course all of them are not exported, but in my doc generator I can decide to generate links for them, based on some schema... links to the external documentation for these libraries...

So if a new ApiItem is created hosting the (not) exported symbol with it's package name, the doc generator can do the magic...

In angular it's a very simple schema: https://angular.io/api/[PACKAGE NAME]/[SYMBOL NAME]
Examples:

import { HttpHeaders } from '@angular/common/http';

https://angular.io/api/common/http/HttpHeaders

import { Component } from '@angular/core';

https://angular.io/api/core/Component

Usually, since documentation is auto-generated, a schema always exists...

alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 7, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 11, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 13, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611
mhevery added a commit to angular/angular that referenced this issue Feb 14, 2019
This enabled dts flattening in the final distrubutable package.

Notes:
 - For the time being this is an opt-in feature via the `ng_module` attribute  `bundle_dts`, however in the near future this will be turned on by default.
 - This only supports the legacy compiler `ngc`, as `ngtsc` emits namespaced imports `import * as __` from local modules which is not supported for the time being by API Extractor. See: microsoft/rushstack#1029

Ref: TOOL-611

PR Close #28588
@IgorMinar IgorMinar referenced this issue Feb 21, 2019
2 of 12 tasks complete
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 21, 2019
At the moment, the API extractor doesn't support local namespaced imports, this will break the generation of flat dts files. When we turn on dts bundling for this package it will break. Hence this is the ground work needed for making this package compatable with the API extractor.

See: microsoft/rushstack#1029

Relates to angular#28588
alan-agius4 added a commit to alan-agius4/angular that referenced this issue Feb 21, 2019
At the moment, the API extractor doesn't support local namespaced imports, this will break the generation of flat dts files. When we turn on dts bundling for this package it will break. Hence this is the ground work needed for making this package compatable with the API extractor.

See: microsoft/rushstack#1029

Relates to angular#28588
IgorMinar added a commit to angular/angular that referenced this issue Feb 22, 2019
At the moment, the API extractor doesn't support local namespaced imports, this will break the generation of flat dts files. When we turn on dts bundling for this package it will break. Hence this is the ground work needed for making this package compatable with the API extractor.

See: microsoft/rushstack#1029

Relates to #28588

PR Close #28642
gkalpak added a commit to gkalpak/angular that referenced this issue Feb 22, 2019
At the moment, the API extractor doesn't support local namespaced imports, this will break the generation of flat dts files. When we turn on dts bundling for this package it will break. Hence this is the ground work needed for making this package compatable with the API extractor.

See: microsoft/rushstack#1029

Relates to angular#28588

PR Close angular#28642
rudolf added a commit to rudolf/kibana that referenced this issue Mar 21, 2019
Becasue of microsoft/rushstack#1029
api-extractor can't use core/index.ts as a single entry point for
analyzing the public and server API's as isolated namespaces.

Instead we analyze these projects separately. This introduces other
problems like the api review files and documentation always being
called "kibana." from the package.json filename.
@jeremymeng

This comment has been minimized.

Copy link
Member

@jeremymeng jeremymeng commented Apr 1, 2019

We are currently using the following pattern. Is there a recommended workaround?

import * as Models from "../lib/generated/lib/models";
...
export { Models };
@octogonz

This comment has been minimized.

Copy link
Collaborator Author

@octogonz octogonz commented Apr 1, 2019

Replying to @shlomiassaf (sorry I didn't see this comment before):

I think that the underlaying issue is more about the concept and direction of this project. [...]
But, for that we need flexibility, understanding that your code base might not be the same as another's code base.

In your example, Angular is one library with a self-contained opinionated representation. Whereas API Extractor's input is a collection of type signatures that are scattered across various .d.ts files, source folders, and external packages, sometimes with different conventions or compiler configurations. API Extractor starts by transforming these inputs, reducing them to a consolidated signature for the library, formulated to be understandable by humans. This is basically what you see in the .d.ts rollup file. The transformation is tricky and somewhat subjective. API Extractor will probably never be able to handle every last exotic TypeScript construct that someone might try to use. However we do aim to support every "reasonable" construct, for example cases that are very popular or don't have easy workaround. In the past year we have made some major steps towards this goal (e.g. enabling support for projects that had no hope of working under API Extractor 6.x). But obviously there's a lot left to do. We'd greatly appreciate community help with that, although the core analyzer code is somewhat complicated and definitely not the easiest area to get started with.

As far as the direction of the project, I'll also mention that we are /not/ seeking to decouple the three use cases (documentation, API reporting, and .d.ts rollups). People frequently ask about this, but these problems are very closely connected, and there lots of advantages to tackling them together. API Extractor 7.x was a big rewrite to make the 3 generators share a common analysis, and it really improved the overall architecture. This "integrated solution" is I believe a major strength of API Extractor compared to other tools.

We are currently using the following pattern. Is there a recommended workaround?

import * as Models from "../lib/generated/lib/models";
...
export { Models };

@jeremymeng Currently the only workaround is to somehow declare your namespaces using the namespace keyword. (I very much want to fix issue #1029, but it's not a simple fix. It may require several days of work or more. I can't tackle that right away, but it's definitely a high priority.)

rudolf added a commit to elastic/kibana that referenced this issue Apr 3, 2019
* Generate core API docs from TSDoc comments

Uses api-extractor and api-documenter to generate documentation for
the Kibana core API from TSDoc comments in the source code.

Documentation can be generated using `npm run docs:api`.

I used --no-verify to ignore the following pre-commit hook errors:
1. Filenames MUST use snake_case - api-extractor.json
   It's possible to specify a different config file, but I prefer to keep the "standard" config file name.
2. UNHANDLED ERROR: Unable to find tsconfig.json file selecting "common/core_api_review/kibana.api.ts". Ensure one exists and it is listed in "src/dev/typescript/projects.ts"
   This is not a source file, so safe to ignore.

* Flesh out API docs a little bit

* Ignore snake_case check for api-extractor.json

* Ignore api-extractor's review file from pre-commit check

* Try to fix build failing by using masters yarn.lock

* I'm being stupid

* Found a better home for ignoring common/core_api_review/kibana.api.ts

* Node script for detecting core API changes

I initially wanted to include this as a precommit hook, but it takes
quite long to execute (~12s) so might be better suited as a test or
as part of the release process.

The script currently fails because api-extractor uses an older version
of typescript.

* Fix tslint precommit hook ignore condition

* Write tsdoc-metadata.json into ./build

* Add LogMeta and ElasticSearch to exported types & docs

* Suppress logging when running api-extractor from script

* Improve check_core_api_changes script and run as test

* Inline api-extractor.json config

* Fix check_core_api_changes --help flag

* LogMeta TSDoc comments

* check_core_api_changes: fail if api-extractor produces warnings or errors

And print more useful messages to the console

* Move ignored ts files list into dev/file

* Add back build:types since api-exporter cannot operate on source files

* Upgrade api-exporter/documenter

* api-extractor: independantly analyze core/public and core/server

Becasue of microsoft/rushstack#1029
api-extractor can't use core/index.ts as a single entry point for
analyzing the public and server API's as isolated namespaces.

Instead we analyze these projects separately. This introduces other
problems like the api review files and documentation always being
called "kibana." from the package.json filename.

* Build types as part of build task

* Include types in typescript browser compilation

* Force inclusion of core/public for building types

* Fix api review filename in api-exporter errors

* Update docs and API review files

* Fix api-extractor warnings

* Remove ts file ignored list since it's no longer necessary

* Rename exported api package name

* Review comments

* Export other missing types

* Upgrade api-documenter to latest beta

* Export more missing types

* Fix warnings and add api-exporter to Jenkins tests

* Correctly handle runBuildTypes() exceptions

* Fix another swallowed exception

* Fix api-extractor warnings after master merge
rudolf added a commit to rudolf/kibana that referenced this issue Apr 3, 2019
* Generate core API docs from TSDoc comments

Uses api-extractor and api-documenter to generate documentation for
the Kibana core API from TSDoc comments in the source code.

Documentation can be generated using `npm run docs:api`.

I used --no-verify to ignore the following pre-commit hook errors:
1. Filenames MUST use snake_case - api-extractor.json
   It's possible to specify a different config file, but I prefer to keep the "standard" config file name.
2. UNHANDLED ERROR: Unable to find tsconfig.json file selecting "common/core_api_review/kibana.api.ts". Ensure one exists and it is listed in "src/dev/typescript/projects.ts"
   This is not a source file, so safe to ignore.

* Flesh out API docs a little bit

* Ignore snake_case check for api-extractor.json

* Ignore api-extractor's review file from pre-commit check

* Try to fix build failing by using masters yarn.lock

* I'm being stupid

* Found a better home for ignoring common/core_api_review/kibana.api.ts

* Node script for detecting core API changes

I initially wanted to include this as a precommit hook, but it takes
quite long to execute (~12s) so might be better suited as a test or
as part of the release process.

The script currently fails because api-extractor uses an older version
of typescript.

* Fix tslint precommit hook ignore condition

* Write tsdoc-metadata.json into ./build

* Add LogMeta and ElasticSearch to exported types & docs

* Suppress logging when running api-extractor from script

* Improve check_core_api_changes script and run as test

* Inline api-extractor.json config

* Fix check_core_api_changes --help flag

* LogMeta TSDoc comments

* check_core_api_changes: fail if api-extractor produces warnings or errors

And print more useful messages to the console

* Move ignored ts files list into dev/file

* Add back build:types since api-exporter cannot operate on source files

* Upgrade api-exporter/documenter

* api-extractor: independantly analyze core/public and core/server

Becasue of microsoft/rushstack#1029
api-extractor can't use core/index.ts as a single entry point for
analyzing the public and server API's as isolated namespaces.

Instead we analyze these projects separately. This introduces other
problems like the api review files and documentation always being
called "kibana." from the package.json filename.

* Build types as part of build task

* Include types in typescript browser compilation

* Force inclusion of core/public for building types

* Fix api review filename in api-exporter errors

* Update docs and API review files

* Fix api-extractor warnings

* Remove ts file ignored list since it's no longer necessary

* Rename exported api package name

* Review comments

* Export other missing types

* Upgrade api-documenter to latest beta

* Export more missing types

* Fix warnings and add api-exporter to Jenkins tests

* Correctly handle runBuildTypes() exceptions

* Fix another swallowed exception

* Fix api-extractor warnings after master merge
joshdover added a commit to rudolf/kibana that referenced this issue Apr 3, 2019
* Generate core API docs from TSDoc comments

Uses api-extractor and api-documenter to generate documentation for
the Kibana core API from TSDoc comments in the source code.

Documentation can be generated using `npm run docs:api`.

I used --no-verify to ignore the following pre-commit hook errors:
1. Filenames MUST use snake_case - api-extractor.json
   It's possible to specify a different config file, but I prefer to keep the "standard" config file name.
2. UNHANDLED ERROR: Unable to find tsconfig.json file selecting "common/core_api_review/kibana.api.ts". Ensure one exists and it is listed in "src/dev/typescript/projects.ts"
   This is not a source file, so safe to ignore.

* Flesh out API docs a little bit

* Ignore snake_case check for api-extractor.json

* Ignore api-extractor's review file from pre-commit check

* Try to fix build failing by using masters yarn.lock

* I'm being stupid

* Found a better home for ignoring common/core_api_review/kibana.api.ts

* Node script for detecting core API changes

I initially wanted to include this as a precommit hook, but it takes
quite long to execute (~12s) so might be better suited as a test or
as part of the release process.

The script currently fails because api-extractor uses an older version
of typescript.

* Fix tslint precommit hook ignore condition

* Write tsdoc-metadata.json into ./build

* Add LogMeta and ElasticSearch to exported types & docs

* Suppress logging when running api-extractor from script

* Improve check_core_api_changes script and run as test

* Inline api-extractor.json config

* Fix check_core_api_changes --help flag

* LogMeta TSDoc comments

* check_core_api_changes: fail if api-extractor produces warnings or errors

And print more useful messages to the console

* Move ignored ts files list into dev/file

* Add back build:types since api-exporter cannot operate on source files

* Upgrade api-exporter/documenter

* api-extractor: independantly analyze core/public and core/server

Becasue of microsoft/rushstack#1029
api-extractor can't use core/index.ts as a single entry point for
analyzing the public and server API's as isolated namespaces.

Instead we analyze these projects separately. This introduces other
problems like the api review files and documentation always being
called "kibana." from the package.json filename.

* Build types as part of build task

* Include types in typescript browser compilation

* Force inclusion of core/public for building types

* Fix api review filename in api-exporter errors

* Update docs and API review files

* Fix api-extractor warnings

* Remove ts file ignored list since it's no longer necessary

* Rename exported api package name

* Review comments

* Export other missing types

* Upgrade api-documenter to latest beta

* Export more missing types

* Fix warnings and add api-exporter to Jenkins tests

* Correctly handle runBuildTypes() exceptions

* Fix another swallowed exception

* Fix api-extractor warnings after master merge
rudolf added a commit to elastic/kibana that referenced this issue Apr 4, 2019
* Generate core API docs from TSDoc comments (#32148)

* Generate core API docs from TSDoc comments

Uses api-extractor and api-documenter to generate documentation for
the Kibana core API from TSDoc comments in the source code.

Documentation can be generated using `npm run docs:api`.

I used --no-verify to ignore the following pre-commit hook errors:
1. Filenames MUST use snake_case - api-extractor.json
   It's possible to specify a different config file, but I prefer to keep the "standard" config file name.
2. UNHANDLED ERROR: Unable to find tsconfig.json file selecting "common/core_api_review/kibana.api.ts". Ensure one exists and it is listed in "src/dev/typescript/projects.ts"
   This is not a source file, so safe to ignore.

* Flesh out API docs a little bit

* Ignore snake_case check for api-extractor.json

* Ignore api-extractor's review file from pre-commit check

* Try to fix build failing by using masters yarn.lock

* I'm being stupid

* Found a better home for ignoring common/core_api_review/kibana.api.ts

* Node script for detecting core API changes

I initially wanted to include this as a precommit hook, but it takes
quite long to execute (~12s) so might be better suited as a test or
as part of the release process.

The script currently fails because api-extractor uses an older version
of typescript.

* Fix tslint precommit hook ignore condition

* Write tsdoc-metadata.json into ./build

* Add LogMeta and ElasticSearch to exported types & docs

* Suppress logging when running api-extractor from script

* Improve check_core_api_changes script and run as test

* Inline api-extractor.json config

* Fix check_core_api_changes --help flag

* LogMeta TSDoc comments

* check_core_api_changes: fail if api-extractor produces warnings or errors

And print more useful messages to the console

* Move ignored ts files list into dev/file

* Add back build:types since api-exporter cannot operate on source files

* Upgrade api-exporter/documenter

* api-extractor: independantly analyze core/public and core/server

Becasue of microsoft/rushstack#1029
api-extractor can't use core/index.ts as a single entry point for
analyzing the public and server API's as isolated namespaces.

Instead we analyze these projects separately. This introduces other
problems like the api review files and documentation always being
called "kibana." from the package.json filename.

* Build types as part of build task

* Include types in typescript browser compilation

* Force inclusion of core/public for building types

* Fix api review filename in api-exporter errors

* Update docs and API review files

* Fix api-extractor warnings

* Remove ts file ignored list since it's no longer necessary

* Rename exported api package name

* Review comments

* Export other missing types

* Upgrade api-documenter to latest beta

* Export more missing types

* Fix warnings and add api-exporter to Jenkins tests

* Correctly handle runBuildTypes() exceptions

* Fix another swallowed exception

* Fix api-extractor warnings after master merge

* Update yarn.lock

* Fix erraneous type

* Revert "Update yarn.lock"

This reverts commit 85a8093.

* Revert "Fix erraneous type"

This reverts commit 7f0550c.

* Backport #32440

* Update core api signature and docs
@gf3

This comment has been minimized.

Copy link

@gf3 gf3 commented May 3, 2019

is there anything we can do to help move this along? we're hoping to use this tool at Shopify to build out some of our public facing documentation for a few libraries and tools

@mterrel

This comment has been minimized.

Copy link

@mterrel mterrel commented Jun 12, 2019

@octogonz After looking at all the similar tools out there for TypeScript projects, this definitely seems like the right choice. But we do use the same pattern as @jeremymeng, so we're blocked by this issue.

I'd rather pitch in and help out here than spend time setting up a different tool, only to come back to api-extractor later. So I'm definitely willing to put in some work to fix this. I've got quite a bit of experience working with the TS compiler API and the AST, but am new to api-extractor. Reading through the previous comments, it sounds like the fix may be a bit involved, so I thought I'd reach out first to see if 1) you'd like some help here and 2) if you've got any high level guidance that you'd like me to follow. I'm happy to jump on Slack or a call or whatever is easiest if you want to chat.

@octogonz

This comment has been minimized.

Copy link
Collaborator Author

@octogonz octogonz commented Jun 13, 2019

@mterrel That would be great and much appreciated! Ping me on Gitter and we can figure something out. Very exciting!

@xirzec

This comment has been minimized.

Copy link
Member

@xirzec xirzec commented Sep 19, 2019

Any chance this is still being worked on? It would be lovely to have for the Azure SDK.

@adventure-yunfei

This comment has been minimized.

Copy link
Contributor

@adventure-yunfei adventure-yunfei commented Sep 23, 2019

Blocking upgrading from AE6 by this, too...

The syntax export * as something from "./localfile works on older api-extractor version (tested on 6.1.3), but fails on latest version (7.4.2).

I remember that I should have fixed this case by #773. Is this a regression caused by AE7 refactor?

@octogonz

This comment has been minimized.

Copy link
Collaborator Author

@octogonz octogonz commented Sep 24, 2019

Is this a regression caused by AE7 refactor?

@adventure-yunfei Yes, in AE6 the .api.ts report was created using the original analysis engine, whereas the .d.ts rollups were created using the newer (more accurate) analysis engine. Thus, in AE6 the export * as something from "./localfile" worked for the API report, but it was broken when generating .d.ts rollups. (The example that you shared does not encounter this problem because .d.ts rollups are disabled.)

In AE7 everything, now uses the newer engine. The older engine is completely removed. That is why this problem has resurfaced. Fixing it is somewhat more difficult, because the solution needs to produce a correct .d.ts rollup.

I don't have time to work on this myself right now (I'm about to start work on some new Rush features), but if someone wants to volunteer, I'd be happy to help with the design.

Here's some initial thoughts.

Interesting edge cases

We need to enumerate the weird edge cases, and come up with a design for how we would represent them in the .d.ts rollup. For example, consider this pathological case:

import * as ns1 from './File1';
import * as ns2 from './File1';
import { thing } from './File1';

export { ns1 };
export { ns2 };
export { thing };

What happens to the string ./File1 when we rolls that up into a single rollup.d.ts file?

It's perfectly okay for our answer to be that "API Extractor supports import * for certain common usages, but certain other usages are NOT supported because (1) they cannot be rolled up reasonably and/or (2) they can be easily rewritten to better forms." It's okay to impose constraints. However, if we do that, it /is/ important to be able to clearly state what is and isn't supported. The rule can't be vague.

This exercise will also lead us to understand how to model ns1. Is it a conventional namespace (i.e. AstNamespace? Or is it some new construct? (These might be easy answers BTW; I haven't had time to think about it in any depth.)

Getting started

With the new analyzer, the starting point is in the _tryMatchImportDeclaration() and _tryMatchExportDeclaration() methods, which parse the import and export statements and decide what to do with them.

The website has some technical notes about the new architecture that might he helpful, along with debugging instructions.

If someone wants to pursue this, ping me on Gitter and I can provide more help.

@adventure-yunfei

This comment has been minimized.

Copy link
Contributor

@adventure-yunfei adventure-yunfei commented Oct 23, 2019

After some investigation, I think namespace or module should be the way. (although namespace should actually be a deprecated feature compared to (external) module).

Syntax for module import

There're two ways to emit .d.ts rollup file for source file:

// foo.ts
import * as bar from './bar';
export { foo }
  1. Use exactly the (external) module syntax:
    // bundle.d.ts
    declare module "<internal-key>/bar" {
      // ...
    }
    import * as bar from "<internal-key>/bar";
    export { bar }
    • pros: keep the same ts syntax node with source file
    • cons: pollute global module declaration
  2. Use similar namespace syntax:
    // bundle.d.ts
    declare namespace bar {
        // ...
    }
    export { bar }
    • pros: keep export/global declaration clean
    • cons: namespace is actually not module

From typescript language, namespace (previously internal module) and module (previously external module) works in very similar way. The second way should be better.

Implementation Step

For .d.ts rollup, there are two phases: analyze (to be included in api-extractor Collector Entities) and output (to *.d.ts bundle).

Analyze Phase

Right now I've made a prototype to first support module import for analyzer (adventure-yunfei#2), and made it work for api json generator (for some demo), by:

  • creating a new AstImportInternal entity. why creating a new one:
    • it should not be another AstSymbol. AstSymbol implies some special logic, e.g. always referenced from outside by root symbol (so always analyze from the top, and only put the top symbol to collector entities))
    • it may be another AstImport, but currently AstImport only indicates external import. So either creating a new kind of entity, or extending AstImport definition.
  • analyze it like AstSymbol (related declarations, file reference, etc), just like a normal local declaration
  • process it in api generator like namespace.

Output Phase

When trying to actually make dts rollup work for module import, I've found many, many edge cases to be handled. So before implementation, I'd first share my syntax propose here.

*.d.ts Rollup syntax propose by namespace:

The idea is:

  • If no module import, generate the same output as before (will not create unnecessary namespace wrapper)
  • If has module import, generate top namespace declaration for each module import. Other standalone declaration for the module content should be merged to that namespace.
  • Won't support default export inside module support (cannot find a way to support it in namespace declaration...)

a) With input-a.ts:

export class Foo {}
  1. and input-index.ts:
    import * as a from './input-a.ts';
    export { a };
    will output:
    declare namespace a {
      export class Foo {}
    }
    export { a };
  2. and input-index.ts:
    import * as a from './input-a.ts';
    export { Foo } from './input-a.ts';
    will output (the same as before):
    declare class Foo {}
    export { Foo }
  3. and input-index.ts (compared to case 2):
    import * as a from './input-a.ts';
    export { a }
    export { Foo } from './input-a.ts';
    will output:
    declare class namespace a {
      export class Foo {}
    }
    import Foo = a.Foo;
    export { Foo }

b) With input-a.ts:

export class Foo {}

and another input-b.ts:

import * as a from './input-a';
export { a }
  1. and input-index.ts:
    import * as b from './input-b';
    import * as a from './input-a';
    export { a, b }
    will output:
    declare namespace a_0 {
      export class Foo {}
    }
    declare namespace b {
      import a = a_0;
      export { a };
    }
    export { a_0 as a }

c) With input-a.ts:

import * as React from 'react';
import { map } from 'lodash';
export { React, map }
  1. and input-index.ts:
    import * as a from './input-a';
    export { a }
    will output:
    import * as React_0 from 'react';
    import * as lodash from 'lodash';
    declare namespace a {
      import React = React_0;
      import map = lodash.map;
      export { React, map };
    }
    export { a }

d) With input-a.ts:

export class Foo {}
export default class Bar {}
  1. and input-index.ts:
    import * as a from './input-a';
    export { a }
    will outout (do not support default import):
    declare namespace a {
      export class { Foo }
    }
    export * as a;

See, the namespace syntax is very complex due to declaration limitation inside namespace.

Note, there seems to be some issues when processing namespace multi-export. (see #1600 found recently)

*.d.ts Rollup syntax propose by module:

Since the original declarations are (external) module, to generate output by module syntax, it's a straight way.

For a complex example, with input-a.ts:

import * as React from 'react';
import { map } from 'lodash';
export class Foo {}

and input-b.ts:

import * as a from './input-a';
export { a }
export default class Bar {}
  1. and input-index.ts:
    import * as b from './input-b';
    export { b };
    export { Foo } from './input-a';
    will output:
    declare module "__internal__/input-a" {
      import * as React from 'react';
      import { map } from 'lodash';
      declare class Foo {}
      export { React, map, Foo }
    }
    
    declare module "__internal__/input-b" {
      import * as a from "__internal__/input-a";
      declare class Bar {}
      export { a }
      export default Bar;
    }
    
    import * as b from "__internal__/input-b";
    import { Foo } from "__internel__/input-a";
    export { b, Foo }

Also, only create module wrapper when required.

For implementation, module syntax should be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.