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

Fix support for baseUrl in typescript projects #244

Closed
antoine-malliarakis opened this issue Sep 18, 2023 · 21 comments
Closed

Fix support for baseUrl in typescript projects #244

antoine-malliarakis opened this issue Sep 18, 2023 · 21 comments
Labels
feature request Feature request

Comments

@antoine-malliarakis
Copy link

antoine-malliarakis commented Sep 18, 2023

Issue description

In the context of our projects heavily rely on the usage of the baseUrl parameter in our typescript configurations.

The issues is that intra workspace package dependencies (e.g. imports from other workspace packages) are not appropriately spotted, leading to project resources invalidly flagged as unused.

@antoine-malliarakis antoine-malliarakis added the feature request Feature request label Sep 18, 2023
@webpro
Copy link
Collaborator

webpro commented Sep 18, 2023

(the idea being that any js file is being refered to directly)

This is the main issue here. Assuming you're in a monorepo, you should not be referencing files in other workspaces directly? But importing them properly using package identifiers and import maps. If you don't want to do that you can still opt into things like ignoreWorkspaces or ignoreDependencies or @public tags.

But maybe I'm missing something? Just guessing based in the input provided.

@antoine-malliarakis
Copy link
Author

(the idea being that any js file is being refered to directly)
This is the main issue here. Assuming you're in a monorepo, you should not be referencing files in other workspaces directly? But importing them properly using package identifiers and import maps. If you don't want to do that you can still opt into things like ignoreWorkspaces or ignoreDependencies or @public tags.

But maybe I'm missing something? Just guessing based in the input provided.

Actually we are importing resources using (classic ?) <package_id>/<path_from_root_of_package> identifiers, example @sorare/core/src/components/card.

From what I can see the ignoreWorkspaces and ignoreDependencies would actually be missing our point since they would be essentially configured to rule out our "shared" packages.

My use case would be essentially the following:

Let's say I have the below structure:

root
\-- packages
     +-- utilitary-package
          +-- package.json
          \-- src
              \-- index.js
     \-- web-app
          + package.json
          \-- src
              \-- index.js

If, in the utilitary-package (referenced from web-app) I define no entry point, only "projects" files, all files will be flagged as unused, even though they could be used by web-app.

The idea with this feature would be to allow such "private" workspace to enlist for being referenced to by other packages from the same workspace.

(To be honest I'm experimenting a lot and trying to tweak configurations so I may have missed something)

@webpro
Copy link
Collaborator

webpro commented Sep 18, 2023

Any chance you could whip up a minimal rep(r)o for this? That would help a lot, then we can take it from there and look at our options.

@antoine-malliarakis
Copy link
Author

antoine-malliarakis commented Sep 18, 2023

Any chance you could whip up a minimal rep(r)o for this? That would help a lot, then we can take it from there and look at our options.

Sure ! Just did it here: https://github.com/antoine-malliarakis/knip-utility-package-reproduction-path

Thanks

P.S.: I already started looking into it if I can help don't hesitate

@webpro
Copy link
Collaborator

webpro commented Sep 18, 2023

Why would you use the empty array for entry: []? Now every workspace does not use any file. My advice would be to remove it.

In case you want to have the util packages not use the default entry file then you can either set that empty array for only that workspace, or use a different file name (i.e. not index.ts).

@antoine-malliarakis
Copy link
Author

Why would you use the empty array for entry: []? Now every workspace does not use any file. My advice would be to remove it.

Actually I only declared it to simplify declaration. It seems that the webapp-a manifest entry paths were actually used, unless I missed something ?

In case you want to have the util packages not use the default entry file then you can either set that empty array for only that workspace, or use a different file name (i.e. not index.ts).

Will be altering the workspace configuration so that it's more explicit.

@antoine-malliarakis
Copy link
Author

@webpro updated the workspace configurations to be compliant with expectations.

@webpro
Copy link
Collaborator

webpro commented Sep 18, 2023

Actually I only declared it to simplify declaration. It seems that the webapp-a manifest entry paths were actually used, unless I missed something ?

I understand the confusion: Knip reads the main field from package.json and also adds that as an entry file. That's not obvious indeed.

For future reference, running with the --debug flag could help. In this case it would show something like this:

[knip] Found entry paths from manifest (packages/webapp-a) (1)
[
  '[...]/packages/webapp-a/index.ts'
]

@homburg
Copy link

homburg commented Sep 18, 2023

We use the same monorepo premise: importing files across workspaces in a monorepo, without package.json:main config.

In my test: homburg/hello-knip#6 knip seems to understand this correctly:

  • An unused library component (Unused.tsx) is marked as unused
  • A used library component (Hello.tsx) is not listed/marked as used

I hope this will be supported in knip in the future as well

@antoine-malliarakis
Copy link
Author

antoine-malliarakis commented Sep 19, 2023

We use the same monorepo premise: importing files across workspaces in a monorepo, without package.json:main config.

In my test: homburg/hello-knip#6 knip seems to understand this correctly:

An unused library component (Unused.tsx) is marked as unused
A used library component (Hello.tsx) is not listed/marked as used
I hope this will be supported in knip in the future as well

By checking your code build logs it seemed to do the trick but when I checked this out this morning it gave me:

Unused files (2)
libs/lib-1/components/Hello.tsx
libs/lib-1/components/Unused.tsx
Unused dependencies (2)
eslint              apps/next-site/package.json
eslint-config-next  apps/next-site/package.json
Unused devDependencies (1)
prettier  package.json

Would need to check what happened in your recent changes.

@homburg
Copy link

homburg commented Sep 20, 2023

Hi @antoine-malliarakis

I can't seem to reproduce your result (Hello.tsx marked as unused) on homburg/hello-knip#6

What commands are you using to run knip? I'm wondering if you are running a particular version

My normal setup for running knip in the repo is:

  • bun install
  • ./node_modules/.bin/knip

@antoine-malliarakis
Copy link
Author

Hi @antoine-malliarakis

I can't seem to reproduce your result (Hello.tsx marked as unused) on homburg/hello-knip#6

What commands are you using to run knip? I'm wondering if you are running a particular version

My normal setup for running knip in the repo is:

  • bun install
  • ./node_modules/.bin/knip

Actually I wasn't using bun. I was using npm. Let me retry with that one.

@antoine-malliarakis
Copy link
Author

@homburg so here we're in an awkward situation. Using bun it works.

Unfortunately I'm not sure that using bun is a viable plan in our build chain (could be checking but I doubt that "just so that knip passes" would be a sufficient motive).

Does the same issue occur for you if you try using npm ?

@homburg
Copy link

homburg commented Sep 20, 2023

@antoine-malliarakis That's interesting :-) I'll try looking into how it works with npm install.

I was using yarn (berry) before, which seemed to work as well

@antoine-malliarakis
Copy link
Author

antoine-malliarakis commented Sep 20, 2023

@antoine-malliarakis That's interesting :-) I'll try looking into how it works with npm install.

I was using yarn (berry) before, which seemed to work as well

To be fair we are using yarn 1. I was trying to use npm because it seemed to me that would be the closest to "bare metal" but then it seems that evidence is proving me wrong. Could that be related to the usage of "workspace:*" ? (We are not using such tricks on our side since we're using yarn 1)

@homburg
Copy link

homburg commented Sep 20, 2023

To be fair we are using yarn 1. I was trying to use npm because it seemed to me that would be the closest to "bare metal" but then it seems that evidence is proving me wrong. Could that be related to the usage of "workspace:*" ? (We are not using such tricks on our side since we're using yarn 1)

An I was naively thinking that the package manager wouldn't affect the result :-) bun was just faster in github actions

@webpro
Copy link
Collaborator

webpro commented Sep 20, 2023

I looked at the repro and the issue is a bug in Knip (or rather a missing feature).

The root cause is setting the baseUrl in tsconfig.json, this results in Knip handling each workspace/project separately. I expected this was already covered, but now I see only the internal dependencies are handled properly, yet not individual files.

Removing the baseUrl is a workaround, but obviously Knip should just deal with that properly. And it may cause other unwanted side effects.

I have some ideas to fix it, will update progress here.

@antoine-malliarakis antoine-malliarakis changed the title Add support for private packages with no explicit entry points Fix support for baseUrl in typescript projects Sep 21, 2023
@antoine-malliarakis
Copy link
Author

Updated description of this ticket following recent findings.

@webpro
Copy link
Collaborator

webpro commented Sep 24, 2023

🚀 This issue has been resolved in v2.26.0. See Release 2.26.0 for release notes.

@webpro
Copy link
Collaborator

webpro commented Sep 24, 2023

Full disclosure. What was not supported yet: having non-entry files being imported from another workspace in the same monorepo AND at least one of the workspaces configured a compilerOptions.baseUrl in tsconfig.json (even if just a "."). That should be fixed now.

@antoine-malliarakis
Copy link
Author

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Feature request
Projects
None yet
Development

No branches or pull requests

3 participants