microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.9k stars 595 forks source link

[api-extractor] Object destructuring causes InternalError #3510

Open Zyie opened 2 years ago

Zyie commented 2 years ago

Summary

I am trying to upgrade from 7.20.1 to 7.28.2 and now I get this error.

InternalError: Internal Error: Unable to determine semantic information for declaration

The line it throws on is const { context } = this;

I've tried every minor version since 7.20.1 and all have the same issue

Repro steps

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
@microsoft/api-extractor version? 7.28.2
Operating system? Mac
API Extractor scenario? rollups (.d.ts)
Would you consider contributing a PR? No
TypeScript compiler version? 4.3.0
Node.js version (node -v)? 16.13.1
zelliott commented 2 years ago

From my brief investigation, it looks like API Extractor is potentially processing your project's .ts files. It should process .d.ts files, not .ts files. But if that was the case, I'd expect the ae-wrong-input-file-type warning to be surfaced. Can you direct me to how API Extractor is configured/invoked on your project?

Zyie commented 2 years ago

Can you direct me to how API Extractor is configured/invoked on your project?

Hey, yeah we have an external repo called @pixi-build-tools/api-extractor-lerna-monorepo that handles the set up so that it runs on each package in a monorepo

It can be found here: https://github.com/ShukantPal/pixi-build-tools/blob/master/packages/api-extractor-lerna-monorepo/index.js

zelliott commented 2 years ago

Thanks!

I added showDiagnostics: true to your Extractor.invoke(...) call and confirmed that API Extractor appears to be running on both .d.ts and .ts files. It should only run on .d.ts files.

Enabling showDiagnostics will print a bunch of diagnostic messages to the console. The section you want to look at is titled "DIAGNOSTIC: Files analyzed by compiler". As you scroll through the list of files, you'll notice some .ts files.

My naive understanding is that this typically means something is configured incorrectly either in your usage of API Extractor or your TS project. I can also take a closer look hopefully in the next few days, but my plate is a bit full at the moment.

Note: I believe the ae-wrong-input-file-type warning wasn't surfaced because API Extractor didn't even successfully complete (sometimes it can still complete despite analyzing .ts files). This is unfortunate... but I remember we purposefully shied-away from throwing an error because we didn't want to break existing projects if they were analyzing .ts files.

EDIT: I think you were probably broken by https://github.com/microsoft/rushstack/pull/3321 (as you said stuff works for 7.20.1 but not 7.21.0). Do you have any tsconfig path mappings in your project?

EDIT 2: Ah, you have the following path mappings set up in your tsconfig:

"paths": {
    "@pixi/*": [
        "packages/*/src"
    ],
    "pixi.js": [
        "bundles/pixi.js/src"
    ],
    "pixi.js-legacy": [
        "bundles/pixi.js-legacy/src"
    ]
},

Before https://github.com/microsoft/rushstack/pull/3321, any imports from these paths would be interpreted as external packages and thus not analyzed by API Extractor. This was a bug. After https://github.com/microsoft/rushstack/pull/3321, these path mappings are now resolved to their underlying mapped paths and analyzed appropriately by API Extractor.

I'll need to spend some more time looking at your project to understand why you're using these path mappings and determine whether this is some reasonable use case API Extractor should support, or if there's something odd about your project.

Zyie commented 2 years ago

Thank you for digging into this, this should help a lot! I'll take a look and see if i can get it to ignore any *.ts files

I'll need to spend some more time looking at your project to understand why you're using these path mappings and determine whether this is some reasonable use case API Extractor should support, or if there's something odd about your project.

We use the path mappings so that individual packages can import other packages by their name e.g. import { Sprite } from '@pixi/sprite'

I believe using path mappings this way is common for monorepo's

alexandressh commented 2 years ago

We use the path mappings so that individual packages can import other packages by their name e.g. import { Sprite } from '@pixi/sprite'

I believe using path mappings this way is common for monorepo's

I'm facing the same issue with version 7.28.4, the error message also points to an object destructuring and I also use those path mappings for easy importing. Downgrading to the version 7.20.1 seems to do the trick.

zelliott commented 2 years ago

Quick disclaimer that I'm really not an export on open source monorepo structures. I have experience working with API Extractor style monorepos but the monorepo of the company I work for is very different from traditional open source monorepos.

When API Extractor runs on each package in a monorepo, it needs to be able to determine which exported declarations are part of that package and which are not. It does this today by looking at import/export module specifiers and resolving their underlying modules. Module specifiers that resolve to some code inside a node_modules/ directory are considered to be external to the package currently being processed, while module specifiers that resolve to code outside of node_modules/ directory are considered to be internal.

In the case of the pixi monorepo, you seem to be using path mappings to simulate importing from external packages. That is, you have something like this:

// bundles/pixi.js/index.ts

...
export * from '@pixi/accessibility';
export * from '@pixi/app';
...

At first glance one might think that these module specifiers resolve to code within node_modules/, but you actually have path mappings set up that map these paths to relative imports. For example:

"baseUrl": "./",
"@pixi/*": [
  "packages/*/src"
],

In API Extractor v7.20.1, we used a hacky heuristic for determining whether a module specifier was external. We basically looked at the module specifier, and if it wasn't relative, we decided it was external. We ignored path mappings entirely. So the re-exports above were determined to be external to the "pixi.js" bundle package, and thus were entirely ignored by API Extractor. This is the reason why API Extractor v7.20.1 doesn't crash on your repo.

In API Extractor > v.7.20.1, we moved to using a TS Compiler API to tell us whether a given module specifier is external. https://github.com/microsoft/rushstack/blob/9fcc2d3c36f1dead30dd88045eab8ad6f3abfd44/apps/api-extractor/src/analyzer/ExportAnalyzer.ts#L258 In particular, see the isExternalLibraryImport usage in the method above. This TS Compiler API evaluates path mappings, and thus considers all of re-exports above to be internal to your current package (i.e. because '@pixi/accessibility' is really './packages/accessibility/src'). This means that they are now processed by API Extractor.

The problem then is that when API Extractor attempts to resolve the module specifiers in your .d.ts file, the TS Compiler API it uses to do so ends up resolving them to .ts source files, not .d.ts files. For example, the resolved module for '@pixi/prepare' ends up being '../pixijs/packages/prepare/src/index.ts. For reference, I'm logging out the resolved module at https://github.com/microsoft/rushstack/blob/9fcc2d3c36f1dead30dd88045eab8ad6f3abfd44/apps/api-extractor/src/analyzer/ExportAnalyzer.ts#L883

We want the resolved module to be .../pixijs/packages/prepare/index.d.ts. And I'm not sure why it isn't... that's what VSCode's "Go to definition" takes us to anyhow. This incorrect .ts resolution is what causes API Extractor to crash - API Extractor should never be processing .ts source files.


So ultimately, where does this leave us:

  1. I still think it's the correct behavior for API Extractor to by default use the isExternalLibraryImport TS Compiler API to determine whether some code is internal or external to the current package. It's more robust than the previous approach.
  2. In your case, I think you want a module specifier like '@pixi/accessibility to be determined to be external. This isn't possible today with how API Extractor treats path mappings. To fix this, either (1) you'll need to remove your path mappings (i.e. use relative imports within a package, absolute imports between packages) or (2) API Extractor needs support for configuring which module specifies are external to your package (i.e. basically some api-extractor.json setting). This is something I've thought about before and would be useful for some of my use cases as well.
  3. Regardless of whether module specifiers are treated as internal or external, API Extractor shouldn't crash. It crashes right now because one of the TS Compiler APIs we're using to resolve modules resolves module specifiers within .d.ts files to .ts source files. I'm not sure whether this is a bug in the TS Compiler API. @andrewbranch you've helped me with questions about getResolvedModule before (https://github.com/microsoft/TypeScript/issues/48807), perhaps you could weigh in here? Happy to open an issue on the TypeScript repo as well.
zelliott commented 2 years ago

@Zyie - JFYI here's one piece of evidence that indicates that perhaps pixi may be misusing path mappings: https://github.com/microsoft/TypeScript/issues/18972. See specifically https://github.com/microsoft/TypeScript/issues/18972#issuecomment-334592613:

the official recommendation for importing local modules is to always use relative paths if the module is going to be published? and just suss out how many ../s you need?

yes. that would be my recommendation.

That would indicate that the right fix here is to remove your path mappings and use relative imports instead.

andrewbranch commented 2 years ago

This is expected behavior for TS. Module resolution isn’t affected by whether the importing file has a .d.ts or .ts extension.

I can’t say whether the paths approach is correct or appropriate for this particular project, but it is pretty common for monorepos that are not a collection of individually publishable packages, but rather a collection of apps and reused libraries that get bundled and deployed as self-contained units. In this model, a bundler would be configured with aliases that mirror the tsconfig paths, so they’re purely an organizational convenience. This is a common pattern in https://nx.dev.

Unfortunately I don’t have specific advice for what API extractor can do in this situation, but happy to answer any more specific questions as you think through it.

zelliott commented 2 years ago

This is expected behavior for TS. Module resolution isn’t affected by whether the importing file has a .d.ts or .ts extension.

Maybe you can help me understand this behavior then. When I'm in bundles/pixi.js/index.d.ts and do "Go to definition" for '@pixi/accessibility', I'm brought to packages/accessibility/index.d.ts.

However, when I change the extension of the originating file to .ts (i.e. bundles/pixi.js/index.ts), and do "Go to definition" for '@pixi/accessibility', I'm brought to packages/accessibility/src/index.ts.

It seems like the file extension is potentially influencing this resolution? I'm wondering if there's a TS Compiler API that can get me the former behavior (right now as described earlier the getResolvedModule API is doing the latter).

andrewbranch commented 2 years ago

It’s hard to tell without inspecting it myself, but it sounds like bundles/pixi.js/index.d.ts may be excluded from the project since it’s an output file, so TS Server isn’t associating it with the project file and so isn’t using paths at all. Go To Definition can also do some mapping magic, so a better way to inspect what’s happening is tsc --traceResolution.

pedroteixeira commented 2 years ago

Is there any way to continue using mapped paths (to avoid relative paths) and avoid these internal errors?

weidezhong-msft commented 2 years ago

In our repo where we build the child pyright repo https://github.com/Microsoft/pyright locally, we have to use compiler's path mapping, otherwise we can't really build the child pyright repo.

PejmanNik commented 1 year ago

I faced the same issue without using the path and by using the baseUrl in the tsconfig and using the absolute path in import for my project file.

The workaround to fix the issue was to use the overrideTsconfig option of api-extractor and change the baseUrl to refer to the declaration output folder; this way api-extractor didn't go to my source files

sedghi commented 1 year ago

Here for the same issue of using path mapping in the tsconfig in a monorepo. After upgrade of the api-extractor i'm seeing the following error, and after adding showDiagnosis I see it is reading from the ts files (because of the path maps)

Error: /Users/alireza/dev/admin/repo/packages/core/src/enums/Events.ts:1:1 - (ae-wrong-input-file-type) Incorrect file type; API Extractor expects to analyze compiler outputs with the .d.ts file extension. Troubleshooting tips: https://api-extractor.com/link/dts-error

API Extractor completed with errors
error Command failed with exit code 1.

https://github.com/cornerstonejs/cornerstone3D/blob/main/tsconfig.json

{
  "extends": "./tsconfig.base.json",
  "compilerOptions": {
    "baseUrl": "./packages",
    "paths": {
      "@cornerstonejs/streaming-image-volume-loader": [
        "streaming-image-volume-loader/src"
      ],
      "@cornerstonejs/core": ["core/src"],
      "@cornerstonejs/tools": ["tools/src"],
      "@cornerstonejs/dicomImageLoader": ["dicomImageLoader/src"],
      "@cornerstonejs/nifti-volume-loader": ["nifti-volume-loader/src"],
    }
  }
}

Should api-extractor add a new configuration to handle monorepos differnetly?

rolandas-valantinas commented 2 days ago

I've encountered same issue while working in NX monorepo. It was correct pointer that api-extractor follows TS paths incorrectly. This is lib after it's built with TSC into dist folder Image api-extractor was running on libraries/ui-tools/dist/src/index.d.ts but that files has export { bob } from '@foo/bar'; Main tsconfig.json has these paths

{
  "@foo/bar": [
    "common/bar/src/index.ts"
  ]
}

So api-extractor was entering libraries/ui-tools/dist/src/index.d.ts and following import into common/bar/src/index.ts instead of going into generated type under common/bar/dist/src/index.d.ts. I've fixed this by creating tsconfig.api.json

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "paths": {
      "@foo/bar": [
        "common/bar/dist/src/index.ts"
      ]
    }
  }
}

And updating api-extractor.json with

{
  "compiler": {
    "tsconfigFilePath": "<projectFolder>/tsconfig.api.json"
  }
}

Bear in mind that you will need api-extractor.json and tsconfig.api.json in each separate project for this to work properly in NX monorepo. I've simplified implementation for the sake of example, but extends can be used for both to centralised common settings and minimise duplication. For completion, target to run api-extractor below

{
  "executor": "nx:run-commands",
  "dependsOn": [
    "build"
  ],
  "options": {
    "commands": [
      "yarn api-extractor run --config {projectRoot}/api-extractor.json"
    ]
  }
}