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

require imports no longer work #681

Closed
ethanneff opened this issue Jun 17, 2024 · 10 comments
Closed

require imports no longer work #681

ethanneff opened this issue Jun 17, 2024 · 10 comments
Labels
regression Something is no longer working

Comments

@ethanneff
Copy link

ethanneff commented Jun 17, 2024

code if (__DEV__) require('../Reactotron/Reactotron');

working in "knip": "5.13.0",

✂️ Excellent, Knip found no issues.

failing in "knip": "5.19.0",

Unused files (2)
src/store/getStoreList.ts
src/packages/Reactotron/Reactotron.ts
Unused exports (1)
storageInstance  unknown  src/packages/Storage/Storage.ts:16:14
@ethanneff ethanneff added the regression Something is no longer working label Jun 17, 2024
@webpro
Copy link
Collaborator

webpro commented Jun 17, 2024

Any chance you could create a reproduction of the issue or add a failing case to one of the test fixtures?

And what's the first version of Knip this fails in for you?

@nihalgonsalves
Copy link

nihalgonsalves commented Jun 18, 2024

It stopped working after 5.16.0. Here's a minimal reproduction:

  • npm init -y

  • entry.ts

    require("./child");
  • child.ts

    function foo() {}
    
    foo();
  • knip.jsonc

    {
      "$schema": "https://unpkg.com/knip@5/schema.json",
      "entry": ["./entry.ts"]
    }
  • Tests:

    $ npx [email protected]
    ✂️  Excellent, Knip found no issues.
    
    $ npx [email protected]
    Unused files (1)
    child.ts
    
    $ npx [email protected]
    Unused files (1)
    child.ts

I couldn't reproduce it with .js files, so maybe that's a hint.

@webpro
Copy link
Collaborator

webpro commented Jun 18, 2024

I couldn't reproduce it with .js files, so maybe that's a hint.

It is. Isn't it odd to require() ts files without an extension? It's not that common, I wonder what tooling/bundler we're using here? And/or what TS config settings?

@nihalgonsalves
Copy link

Hmm I don't think it's unusual. Using tsc and running the output with node was the default basic Node + TypeScript setup (on the backend) until pretty recently.

I suppose using require with that setup is perhaps uncommon, as tsc does compile import statements for CJS. The case I ran into was using an entrypoint with require('reflect-metadata'); require('./app'); (I was able to change the knip entry as a workaround).

@webpro
Copy link
Collaborator

webpro commented Jun 21, 2024

I really need an actual reproduction of the issue. The basics work, I just added 0c56610, perhaps its specific TS config settings?

@nihalgonsalves
Copy link

Does my previous reproduction not work? That was without any tsconfig in the directory at all – just those three files and the default package.json from npm init -y

@webpro
Copy link
Collaborator

webpro commented Jun 21, 2024

Copy-pasting something here is not a reproduction. No tsconfig.json is most likely the problem. In isolation, why would you expect that to work? Without context I don't understand the situation/use case here.

@nihalgonsalves
Copy link

I was trying to make it as minimal as possible .. wouldn't tsconfig.json just imply some default settings anyway?

Here's my initial reproduction with a bare tsconfig.json added; it doesn't seem to make a difference to the knip output: https://codesandbox.io/p/devbox/knip-issue-681-xkv92z

I'm not sure I understand why you wouldn't expect that to work? It's a valid program that can be run via tsc and then node on the output. I added those scripts to the sandbox too.

@webpro
Copy link
Collaborator

webpro commented Jun 21, 2024

Thanks for the reproduction.

The underlying issue is that require() uses node dist/entry.js from the package.json#main as a starting point. And dist/*.js is also part of the project files by default, you probably don't want that.

So my recommendation would be to split up src and dist directories, eg:

{
    "$schema": "https://unpkg.com/knip@5/schema.json",
    "entry": ["src/entry.ts"],
    "project": ["src/*.ts"],
}

Not sure if this also is the "fix" for OP, but based on this repro I think the refactoring I did more reveals an issue in cases like this than that it introduces a regression. That split is also better for performance, as Knip does/should not wander off into compiled build artifacts.

@webpro
Copy link
Collaborator

webpro commented Jul 12, 2024

Since OP didn't provide a reproduction and I believe the repro that was provided is handled properly I'm going to close this issue.

@webpro webpro closed this as completed Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Something is no longer working
Projects
None yet
Development

No branches or pull requests

3 participants