webpro-nl / knip

✂️ Find unused files, dependencies and exports in your JavaScript and TypeScript projects. Knip it before you ship it!
https://knip.dev
ISC License
7.06k stars 177 forks source link

🐛 Not following `references` in TS config files #779

Closed Jym77 closed 2 months ago

Jym77 commented 2 months ago

Prerequisites

Reproduction url

Code Sandbox

Reproduction access

Description of the issue

I have a TS package with a src and test directories. Files in src are built to dist (which is then included in the package's files entry). Files in test are built there (and not exported). This implies one tsconfig.json in each of these directories.

The src/foo.ts file creates some internal exports:

/** @internal */
export const foo = 1

In real life, this is typically used for stuff that I do not want to put in the package's API but I want to be able to test and thus import from the test files.

The test file then include these as import { foo } from "./dist/foo.js". This is the correct import since TS want them to point to ~the resulting file~ the build artefact.

However, knip reports this as unused:

Unused exports (1)
foo  unknown  src/foo.ts:2:14

When the test file instead import { bar } from "../src/foo.js" (which is incorrect and creates a runtime error), knip does not report it anymore.

So, it seems that knip is not correctly linking the source file with the generated code.


Note: when building the repro, initially import { bar } from "../src/foo.js" was creating a build error. Only after removing the outDir option, building, then setting it up again (and cleaning) did I land in this situation (no build error, runtime error).

webpro commented 2 months ago

Knip applies some "source mapping" when the config allows so: https://knip.dev/features/monorepos-and-workspaces#source-mapping to analyze only source files.

It's not actually a monorepo feature, it also applies in your case.

The test file then include these as import { foo } from "./dist/foo.js". This is the correct import since TS want them to point to the resulting file.

This might be the culprit here. In general you don't want to import "resulting" files from tests or source file. Do you mean build artifacts? And maybe you're referring to import specifiers that have the .js extension instead of the actual file's .ts extension?

Jym77 commented 2 months ago

The test file then include these as import { foo } from "./dist/foo.js". This is the correct import since TS want them to point to the resulting file.

This might be the culprit here. In general you don't want to import "resulting" files from tests or source file. Do you mean build artifacts? And maybe you're referring to import specifiers that have the .js extension instead of the actual file's .ts extension?

Yes, I meant the build artefact.

TS requires me to use the .js extension in its imports, not .ts (and to point to the correct file). I can't claim I really understand why at the deepest level, but there are numerous discussions:

TS documentation

Recall that when specifying a file extension on a relative module specifier, TypeScript typically makes you use the output file extension

https://github.com/microsoft/TypeScript/issues/55346 or https://github.com/microsoft/TypeScript/issues/59597#issuecomment-2284566272

webpro commented 2 months ago

TS requires me to use the .js extension in its imports, not .ts (and to point to the correct file). I can't claim I really understand why at the deepest level, but there are numerous discussions:

Target the actual existing TS source file, and replace the .ts extension with .js. The import specifier should not point to the build artefact (which might not even exist yet).

Jym77 commented 2 months ago

That, at least, does not work when verbatimModuleSyntax is true, as recommended for libraries (and actually most cases on that page).

In the code box, test/foo.spec.ts contains:

import { foo } from "../dist/foo.js";
import { bar } from "../src/foo.js";

console.log(foo + bar);

Which is left essentially untouched in test/foo.spec.js, thus resulting in a runtime error Cannot find module '/project/workspace/src/foo.js' when trying to node test/foo.spec.js.

This may also depends on ESM vs CJS (this is ESM), and of module (here Node16, which is also the recommended setting).


From what I understand of Module specifiers are not transformed, it looks like the import string is never ever transformed, so it should always point to the build artefact even before it exists.

There is no compiler option that enables transforming, substituting, or rewriting module specifiers. Consequently, module specifiers must be written in a way that works for the code’s target runtime or bundler, and it’s TypeScript’s job to understand those output-relative specifiers.

(emphasis not mine)

webpro commented 2 months ago

Module specifiers are not transformed so they still work after compilation.

Jym77 commented 2 months ago

Yes, that a bit the point 😅 Not sure where we're getting 😕

Am I missing something? 🤔

According to knip source mapping:

If src/index.ts exists, Knip will use that file instead of dist/index.js.

But this is not what I'm seeing since foo is reported as unused export. (the example in the doc has a baseURL but adding it does not change the behaviour). Even more confusing, the incorrect import { bar } from "../src/foo.js" is not reported.

This may be linked to the directories structure I have with a top-level tsconfig.json, and extending it in each of src and test (since I do not want outDir: "dist" for the tests, in order to keep them out of the distributed package).

webpro commented 2 months ago

Am I missing something? 🤔

Yes, don't import the file at dist/foo.js but src/foo.js

But this is not what I'm seeing since foo is reported as unused export.

Happy to discuss after we've sorted the first point. But I can imagine there'll be issues since Knip doesn't actively support references or tsconfig.json files that are not in the workspace root.

Jym77 commented 2 months ago

Am I missing something? 🤔

Yes, don't import the file at dist/foo.js but src/foo.js

But that just plain out doesn't work (from the TS/node point of view). There is no file at src/foo.js, and there will never be any, since I told TS to build directly to dist ("outDir": "./dist" in src/tsconfig.json).

import { bar } from "src/foo.js" creates a runtime error when I try to run the resulting file (node test/foo.spec.js).

But I can imagine there'll be issues since Knip doesn't actively support references or tsconfig.json files that are not in the workspace root.

I guess that's where the problem is. Then knip doesn't see the outDir (which is not in the workspace root, since I want test files to not go to dist and be included in the package) and indeed doesn't resolve dependencies correctly. The workspace root TS config does have a reference to the src one, but if knip doesn't support it…

webpro commented 2 months ago

But that just plain out doesn't work (from the TS/node point of view).

Maybe it "doesn't work" in the situation you've created.

There's tools like ts-node, tsx, ts-jest, jiti etc. to do the compilation during runtime (so you don't need to compile upfront). Or compile the whole thing first using e.g. tsc or swc and then the result in dist is ready to run in Node.js.

Indeed, the file doesn't exist. Perhaps another thing to "show it works": if you use ./foo.js as import specifier while there's only foo.ts you can still opt-click on it and an IDE like VS Code or WebStorm will go to foo.ts. This is how TypeScript module resolution works (but Node.js module resolution doesn't support this indeed).

Jym77 commented 2 months ago

But that just plain out doesn't work (from the TS/node point of view).

Maybe it "doesn't work" in the situation you've created.

Well, … yes… What I've created is perfectly fine for TS and node. You can run yarn tsc --build and node test/foo.spec.js and it works fine (well, not really, the import { bar } from "../src/foo.js" does not work, but the import { foo } from "../dist/foo.js" does work).

In this setup, the only correct (from TS / node point of view) import is from ../dist/foo.js.

However, knip does not recognise it. Because it does not read the src/tsconfig.json file that is not at the workspace root but merely referenced by it.

What I assume (maybe wrongly), is that this situation I've created is not inherently wrong and should be handled correctly by knip. (what I initially misunderstood was the root cause, it now seems to me that this is due to not handling references)

There's tools like ts-node, tsx, ts-jest, jiti etc. to do the compilation during runtime (so you don't need to compile upfront).

I should not be forced to used another tool 😄 (and runtime compilation is not a good idea to ship a library, I want to be able to pack ES files).

Or compile the whole thing first using e.g. tsc or swc and then the result in dist is ready to run in Node.js.

Which is what I'm doing. I'm compiling with tsc. This puts the output in dist/foo.js. This means that the only correct (from TS / node point of view) way to import it is through ../dist/foo.js, not through ../src/foo.js as you mentioned in https://github.com/webpro-nl/knip/issues/779#issuecomment-2337934349

knip doesn't follow the references; thus it doesn't see src/tsconfig.json; thus it doesn't see that dist/foo.js is built from src/foo.ts; thus it incorrectly reports export foo (in src/foo.ts) as unused because it sees only the one from dist/foo.js being used.

webpro commented 2 months ago

Not sure what to tell you. You're saying "I can't claim I really understand why at the deepest level" and then insist on doing things this way.

It's kinda odd that the test files are compiled into the same directory, and then the resulting JS files are used to exercise the compiled source files. Overall this isn't a best practice and a bit of a mess in the CSB repro. Test runners like Vitest, Bun (or even ts-jest) are there to take the burden away in case the test (and source) files are written in TypeScript. There is no need and there should not be a need for a compilation step of test files. So either everything should be compiled upfront if you insist on running things directly in Node.js without any dependencies. Or, I'd recommend, use a test runner that supports TypeScript.

Jym77 commented 2 months ago

Not sure what to tell you. You're saying "I can't claim I really understand why at the deepest level" and then insist on doing things this way.

The part I don't fully understand is why TS cannot rewrite module specifiers. Which I do not have a choice of doing things any other ways.

It's kinda odd that the test files are compiled into the same directory, and then the resulting JS files are used to exercise the compiled source files. Overall this isn't a best practice and a bit of a mess in the CSB repro. Test runners like Vitest, Bun (or even ts-jest) are there to take the burden away in case the test (and source) files are written in TypeScript. There is no need and there should not be a need for a compilation step of test files. So either everything should be compiled upfront if you insist on running things directly in Node.js without any dependencies. Or, I'd recommend, use a test runner that supports TypeScript.

Well, I've inherited the repo with a simple test setup not using any runners (granted, it was set up in the early days of TS and TS aware test runners might not have been that common back then). Not enough time or resources to re-architect this when it is effectively working (tests are run, my code is exerciced, bugs and regressions are detected), and even gets a few benefits (low startup time for tests, easy to manually run a single test file for super quick iterations).

Anyway, the same situation can be built without tests. I've updated the sandbox to have several directories inside src that are then built into separate dist-* directories. Arguably a contrived setup, but not a broken one (from TS + node point of view). Maybe not very realistic, I agree, whatever it tries to do might probably be better handled by exports field in the manifest file, or through proper workspaces.

In any case, I know have src/tsconfig.json that uses references to point to the sub-directories. From src/index.ts, I need to import { foo } from "../dist-foo/foo.js". This is the only correct module specifier and any other will break at runtime. But knip does incorrectly report src/foo/foo.ts as an unused file.

If I replace the import by import { foo } from "./foo/foo.js", which looks correct under the assumption that all of src will be built into the same directory without changing the structure, then knip stops complaining. But that module specifier is wrong an will result into a broken JS file that will crash at runtime.

So, knip not following references in TS config files can lead to ignoring some of them and thus report false positives.

webpro commented 2 months ago

Guess I steered the discussion sideways myself here, yet I'm also not the one to tell how to organize any project. Still, the repro doesn't meet the requirements:

That’s why it’s required to provide a minimal reproduction. This contains only the source code and configuration required to demonstrate the issue

I'm going to close this one as it's a bit convoluted. I'm happy to discuss the actual issue at hand in a new issue/repro.