Skip to content

Conversation

@zelliott
Copy link
Contributor

@zelliott zelliott commented Apr 9, 2022

Summary

Background

Before #3321, API Extractor always treated ambient modules as "external" (unless they were specified as bundled packages, but this would be a misuse of that configuration). This meant that API Extractor would never attempt to resolve an ambient module with ts.getResolvedModule, and thus no errors would be thrown (as ts.getResolvedModule returns undefined for ambient modules, and API Extractor throws if ts.getResolvedModule returns undefined).

(Note that it's not correct to always treat ambient modules as "external", it's possible for a project to have ambient modules within it, API Extractor just didn't support that use case yet.)

After #3321, API Extractor now uses the ts.getResolvedModule helper to determine whether a module is "external". This means that API Extractor also uses this helper to determine if ambient modules are external, which unfortunately doesn't work as stated above. The end result of this was that ts.getResolvedModule would return undefined for ambient modules, and then we'd consequently throw an InternalError. This is the root cause of the build breakage at #3338.

Fix

The fix in this PR is to simply revert to how API Extractor previously handled ambient modules. We know that ts.getResolvedModule returns undefined for ambient modules. Thus, within _isExternalModulePath, simply return
true if ts.getResolvedModule returns undefined. This results in all ambient modules being treated as external, which
matches API Extractor's previous behavior.

This PR also altogether removes the pathMappings test scenario. This is because this test scenario is not working as expected - in short - the TS compiler used by API Extractor is resolving statements such as export { AbstractClass } from 'path-mapping'; to .ts files instead of .d.ts files. You can tell this is the case by looking at some of the artifacts generated by API Extractor for that test scenario and noticing that they include syntax from .ts files. I think there's some issue with how the path mappings are breaking the generated .d.ts file import/export statements. I'll need more time to look into this, and the test scenario isn't accurate as-is, so I removed it.

Follow up tasks

  • We need to better understand (1) when ts.getResolvedModule returns undefined and (2) when isExternalLibraryImport is undefined. Can the TypeScript team help with this? There's little to no documentation about this helper.
  • We still need to fix [api-extractor] API Extractor cannot resolve ambient modules #3335. This issue existed before [api-extractor] API Extractor now properly runs on projects with tsconfig path mappings. #3321 and still exists after. We'll need to ask the TypeScript team how to resolve ambient modules if ts.getResolvedModule can't be used.
  • We need to add test cases for both (1) ambient modules and (2) path mappings. Both of these test cases will probably need to be in standalone build-tests sub-directories instead of within the api-extractor-scenarios test case as before, as they require more involved project setup.

How it was tested

@octogonz
Copy link
Collaborator

octogonz commented Apr 9, 2022

✔ I confirmed that this patch fixes the error in PR #3338, unblocking the upgrade for the Rush Stack repo.

✔ I also tested this branch against our large monorepo at work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants