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 "export { x }" for converting a source file's exports into a named module #663

Closed
donahut opened this issue May 19, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@donahut
Copy link

commented May 19, 2018

Given an index.ts such as:

import * as d from './d';

export * from './A';
export * from './B';
export * from './C';

export {
    d
};

The generated .apt.ts only produces valid output for A, B and C, whereas d produces things like:
// WARNING: The name ""<path>/lib/dts/d/index"" contains unsupported characters; API names should use only letters, numbers, and underscores
and
module "<path>/lib/dts/d/index" {

This sort of local namespacing and export pattern is used throughout our codebase, so any workarounds would be welcome.

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2018

This should get fixed during June as part of our sprint to convert the *.api.ts files to be generated using the new analysis engine that was created for the *.d.ts rollups.

@adventure-yunfei

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

@pgonzal is this sovled now? still get this error.

@octogonz octogonz self-assigned this Sep 14, 2018

@octogonz octogonz changed the title Namespaced Imports/Exports break extraction [api-extractor] Namespaced Imports/Exports break extraction Sep 14, 2018

@southpolesteve

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

I ran into this issue too. Was able to work around a by refactoring some of our exported types.

@octogonz octogonz changed the title [api-extractor] Namespaced Imports/Exports break extraction [api-extractor] Support "export { x }" for converting a source file's exports into a named module Nov 1, 2018

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 1, 2018

I had actually not looked very closely at the original example. There are two unsupported things going on here:

// 1. Converting a source file's exports into a named module
import * as d from './d';

// 2. Exporting a symbol that was locally defined earlier in the same file
export { d };

Thanks for bringing this to my attention. I've actually never seen this pattern in real usage before heheh. From a "TypeScript: the good parts" mindset, it strikes me as slightly odd: We tend to take the perspective that when you write a declaration, you are defining an API, versus defining a piece of plumbing that other files might combine to produce the final API. This philosophy has several practical benefits:

  • Our own unit tests and internal code should use the API in the same way that a third-party developer would use it.
  • We try to avoid giving two different names to the same type. It would be confusing. For example, it would make it harder for people to search a large code base to find all references to a given declaration, which is a pretty important scenario.
  • API definitions should correspond to a small palette of common stereotypes, that are easy for people to recognize, and that the documentation system can represent nicely.

For the case of export { d };, it's unclear where the doc comment for d should even go. The places I can think of (above 1, above 2, or as a first doc comment in the original source file) all have technical complexities due to how the parser handles "trivia" between ast nodes.

That said, I believe that the .d.ts rollup engine should support this feature. Right now API Extractor fails hard with Error: Unsupported export: blah on this input, which is not expected behavior.

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2018

The fix for this is related to Issue #949 . We'll tackle them together after AE7 is released.

@adventure-yunfei

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

@pgonzal I thought this issue is fixed by #773, still not working? I'm using this syntax and it works fine for me. (tested with lastest version - api-extractor@6.1.6 and api-documenter@1.5.56)

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 26, 2018

I thought this issue is fixed by #773, still not working? I'm using this syntax and it works fine for me. (tested with lastest version - api-extractor@6.1.6 and api-documenter@1.5.56)

@adventure-yunfei are you using .d.ts rollups? Or just generating API documentation? That might be the difference.

This sort of local namespacing and export pattern is used throughout our codebase, so any workarounds would be welcome.

@donahut BTW the workaround is pretty easy. For example, instead of this:

d.ts

  /**
    * (Doc comment for Example) 
    * @public 
    */
export class Example { }

index.ts

import * as d from './d';

// (doc comment won't be recognized here)
export { d };

...you could replace it with this:

d.ts

/**
  * (Doc comment for d) 
  * @public 
  */
export namespace d {
  /**
    * (Doc comment for Example) 
    * @public 
    */
  export class Example { }
}

index.ts

export { d } from './d';
@adventure-yunfei

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

@adventure-yunfei are you using .d.ts rollups? Or just generating API documentation? That might be the difference.

Reproduced by myself. As you described, when enable "dtsRollup", it's not working... Seems I didn't consider this case.

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 26, 2018

In API Extractor 7 all the features (docs, API reviews, and .d.ts rollups) will use the same analysis engine. So we really need to prioritize this fix.

@adventure-yunfei

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

Tested with AE7, this import * as foo from '..' and re-export case is not supported (as the AE6 standalone documentation generator is gone), which becomes a block issue for my project.

I've tried to investigate. There're two slightly difference cases that are not supported now:

  • case 1: import * from external package/module:
import * as semver from 'semver';
export { semver };
  • case 2: import * from local module:
import * as local from './local';
export { local };

For case 1, the error is Error: Child declaration not found for the specified node, because the AstSymbol (returned from SymbolAnalyzer. followAliases) is ts SourceFileObject and thus set as "nominal" (in AstSymbolTable._fetchAstSymbol), and then we skip it in AstSymbolTable.analyze. So declarations inside that SourceFileNode is not analyzed.

For case 2, the error is Error: Unsupported export: blah..., because in SymbolAnalyzer.followAliases, the symbol is falsely checked as isAmbient (see code SymbolAnalyzer.ts#L142, the node is exactly kind of ts.SyntaxKind.SourceFile, but when searching we skip this start node).

Another possible problem of case 2 is that in SymbolAnalyzer. _followAliasesForImportDeclaration, inside the _getPackagePathFromModuleSpecifier check we skip the local module import. I tried to make this check pass and return the absolutely local module path, but then the case falls back to the error of case 1.

From the code I see that the ts SourceFileObject type is currently treated as a special case. Seems a tough case for me to solve. Perhaps I need more time to study the new .d.ts rollup engine.

Could you please let me know whether someone is fixing this? If so, I'll wait for your good news and keep using AE6 (with my own fix) for now. If not, I may spend more time later to solve this blocking (for us) issue.

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 30, 2018

I would love to get some community help with fixing various edge cases for the AE7 beta.

However, I feel like this particular issue may be difficult for an external contributor to fix, since it requires a design change for the SymbolAnalyzer. I was planning to fix this myself, but first I need to work through a small punch list of other issues that are blocking our own projects from adopting AE7 (which is basically our bar for the first non-beta release of AE7).

If you're completely unable to continue testing AE7 without this fix, let me know and I can try to prioritize it sooner.

@adventure-yunfei

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

Thanks for quick response. Generally this issue won't actually affect my project, since I can continue to use AE6 (and yes if switching to AE7, this is a blocking issue for me). So it's fine to continue your plan for the first AE7 release.

The current situation is, with my own fix for AE6 (including the closed #964), I can successfully generate an api document site which at least contains all exported items. The next feature I'm trying to do is enhance the type value link (e.g. the link for IAadHttpClientConfigurations of property configurations from document https://docs.microsoft.com/en-us/javascript/api/sp-http/aadhttpclient?view=sp-typescript-latest#configurations , or the type link for type alias declaration). Before AE7 I actually didn't take much attention to the .d.ts rollup engine (as api document is our first mission). So I may first take time to see what's changing now.

By the way, I'm coming from a company and heavily take advantages of typescript (to build many 3D graphics related web apps). Cause of complex business logic, we indeed used some advanced features (or almost every possible one) of typescript. Since the code is huge now, we need such typescript related infrastructures. This project means too much for us! We're happy to take some time (but may be not much) to make some contribution to the growing typescript/tsdoc ecosystem!

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Jan 17, 2019

The following declaration types are now supported after PR #1002 was merged:

  • "export { x }", including exporting the same symbol twice with two different names
  • import * as someName from 'external-package';

This declaration type is still not implemented yet:

  • import * as someName from './localFile';

I've opened a separate issue #1029 to track that. Thus I am closing this issue.

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