Skip to content

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

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

Closed
donahut opened this issue May 19, 2018 · 13 comments
Assignees
Labels
blocked We can't proceed because we're waiting for something bug Something isn't working as intended effort: medium Needs a somewhat experienced developer

Comments

@donahut
Copy link

donahut 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
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link

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
Copy link
Collaborator

octogonz 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 octogonz added bug Something isn't working as intended effort: medium Needs a somewhat experienced developer blocked We can't proceed because we're waiting for something labels Nov 22, 2018
@octogonz
Copy link
Collaborator

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

@adventure-yunfei
Copy link
Contributor

@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 - [email protected] and [email protected])

@octogonz
Copy link
Collaborator

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 - [email protected] and [email protected])

@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
Copy link
Contributor

adventure-yunfei 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
Copy link
Collaborator

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
Copy link
Contributor

adventure-yunfei 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
Copy link
Collaborator

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
Copy link
Contributor

adventure-yunfei 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
Copy link
Collaborator

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
Labels
blocked We can't proceed because we're waiting for something bug Something isn't working as intended effort: medium Needs a somewhat experienced developer
Projects
None yet
Development

No branches or pull requests

4 participants