Skip to content
This repository has been archived by the owner on Mar 10, 2024. It is now read-only.

Don't mark entry files as unimported #72

Closed
dbartholomae opened this issue Jan 30, 2022 · 12 comments
Closed

Don't mark entry files as unimported #72

dbartholomae opened this issue Jan 30, 2022 · 12 comments

Comments

@dbartholomae
Copy link
Contributor

Not sure if I just misunderstood something about the configuration, but the way that I understand it, I have to mark tests as entry files, or otherwise test helpers and test dependencies will not be recognized as being imported. But it seems I need to still mark the test files as ignored as well. At the same time, e. g. the pages in my next app are not ignored, but they do not show up in the unimported output.
Here's my config for reference:

{
  "ignorePatterns": [
    "**/node_modules/**"
  ],
  "ignoreUnimported": [],
  "ignoreUnused": [
    "next",
    "react",
    "react-dom"
  ],
  "ignoreUnresolved": [],
  "entry": ["pages/**/*", "src/**/*.stories.tsx", "src/**/*.test.ts", "scripts/*.ts"]
}
@smeijer
Copy link
Owner

smeijer commented Jan 30, 2022

Tests files, as well as the helpers, should be added to the ignore patterns. Unless you indeed also want to verify those, then you'll need to specify the tests as entry point. I wouldn't recommend that.

Test dependencies should be installed as devDependency, and as devDependencies are ignored by default, they won't be reported.

@dbartholomae
Copy link
Contributor Author

So should I also ignore all test helper files? And what if some utils are used in test helpers and in production code because they are helpful in both? Then they might show up as unused when the production code no longer uses them, get deleted, which breaks tests, leading to a hunt for what actually is used.
To be honest, treating test code not with the same rigor as production code sounds problematic to me.
Also, it is just conceptually confusing that some files in entry are automatically ignored, and others are not. I have no clue which ones are which.

@smeijer
Copy link
Owner

smeijer commented Jan 30, 2022

Yes, if it's really a test helper, then it should be marked as ignored. If the helper is also used in production, then it's no longer a test helper.

Unimported is currently focused around production/application code, and not around the tests. You could indeed specify all tests (via pattern) as entry file and then you get the behavior as you desire.

I get that it might be confusing. We do some detection to determine the type of project, and optimize the setting so that only application/production files are part of the report.

To make it work correctly for tests, we need a change to also include devDependencies though. And then we need to analyze if a imported dependency is rightfully declared as dependency, or should be moved to devDeoendencies.

That was the original purpose of unimported. Analyze the code base, and tell which dependencies can be uninstalled to reduce the install size.

I'm happy to accept a pull request that adds support for your use case. But I also do want to keep support for the current one. Ideally, I think we need to make it run twice, once for application code, and once for test code, and then do an intersection.

@dbartholomae
Copy link
Contributor Author

I'm happy to provide a PR, though I would like to clear up the solution to this first. From my perspective, the easiest way would be to move the special logic that determines entry points into the creation of the config file, so that everything is transparent from the config file, and then to just automatically ignore all entry files. What do you think?

@smeijer
Copy link
Owner

smeijer commented Jan 31, 2022

Entry files are already ignored in that perspective, as they're always imported. They can't be reported as it is right now.

The feature that I believe that you need, is that we support multiple "environments" or "modes"

Basically we need a way to:

  • analyze the application code (current way of operating)
  • analyze the test/dev code and thereby devDependencies
  • run both those modes, and report the intersection

There is no bug in the current way of reporting things. To support your case, we need to add a "mode switch" that makes unimported analyze test files instead of application files, and thereby also using devDependencies instead of dependencies.

When running unimported in both modes, and printing the intersection, we have the functionality that you, to my understanding, desire.

@dbartholomae
Copy link
Contributor Author

For me, adding a file to entry does not mark them as imported. Here's a reproducing repo:
https://github.com/dbartholomae/unimported-bug

In that repo, running npx unimported yields
image
even though I have

{
  "entry": [
    "src/App.test.tsx",
    "src/index.tsx"
  ]
}

Maybe related to me working on Windows?

@dbartholomae
Copy link
Contributor Author

Yes, it's related to Windows:
https://github.com/dbartholomae/unimported-bug/runs/5058313575

@Cigan12
Copy link

Cigan12 commented Feb 8, 2022

image
Same here, on windows

@smeijer
Copy link
Owner

smeijer commented Feb 8, 2022

@Cigan12 what happens if you remove the src segment from the entry files?

{
  "entry": ["./client/src/main.tsx", "./server/src/main.ts"]
  ...

@smeijer
Copy link
Owner

smeijer commented Feb 8, 2022

I don't own a Windows machine at the moment. So if one of you could submit a fix, that would be immensely appreciated.

@dbartholomae
Copy link
Contributor Author

I'm happy to take a look, but currently I can't even install due to the following error:

ERROR Failed to apply patch for package flow-remove-types at path

@smeijer
Copy link
Owner

smeijer commented May 29, 2022

Windows issues have been fixed in 1.20.1, so I'm gonna assume that this one is also no longer the case. Please reopen if I'm wrong.

@smeijer smeijer closed this as completed May 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants