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

Ensure that @types/* used in emitted declarations are in dependencies #248

Closed
alecmev opened this issue Sep 18, 2023 · 14 comments
Closed

Ensure that @types/* used in emitted declarations are in dependencies #248

alecmev opened this issue Sep 18, 2023 · 14 comments
Labels
feature request Feature request

Comments

@alecmev
Copy link

alecmev commented Sep 18, 2023

I'm not sure if this is in scope of Knip, but it feels like it could be. In a nutshell, if I have a module like this:

// bar/src/index.ts
import type { Foo } from "foo";
export const bar = (): Foo => ...

And foo comes with a corresponding @types/foo, then I need to make sure that it's actually in dependencies. Otherwise bar/dist/index.d.ts will be broken in any consumer of bar that doesn't have @types/foo in its dependency tree.

The complication I'm seeing is determining whether a @types/* actually ends up being used in dist. And putting all @types/* in dependencies doesn't make sense, obviously. Does TypeScript have an API of any sort for checking this?

More context in this Stack Overflow question.

@alecmev alecmev added the feature request Feature request label Sep 18, 2023
@webpro
Copy link
Collaborator

webpro commented Sep 18, 2023

Yes, I think Knip can help here. Related to #241.

@webpro
Copy link
Collaborator

webpro commented Sep 27, 2023

Currently Knip just expects @types/* packages to be in devDependencies (with --production --strict).

So yeah. the issue is that @types/pkg sometimes need to be in dependencies.

More refs:

To be continued

@webpro
Copy link
Collaborator

webpro commented Sep 27, 2023

Any chance you could whip up a repro? One or more cases would be very helpful. Running in strict production mode should "fail" now.

@alecmev
Copy link
Author

alecmev commented Sep 27, 2023

Spent half an hour trying to understand why I can't get my repro to fail, when I realized that Knip actually never complained 😂 Knip is fine with @types/* being anywhere, or missing. This repro currently passes, but needs to fail.

@alecmev
Copy link
Author

alecmev commented Sep 28, 2023

Updated the repro to also include a case without @types/* (it shouldn't report @microsoft/tsdoc as unused).

@danvk
Copy link

danvk commented Sep 29, 2023

@alecmev you can often take advantage of duck typing to sever @types dependencies, i.e. define an interface with just your needs in your public API that's compatible with the other package's type. Just doing my part to fight dependency bloat :)

If you know examples of popular npm packages that bundle TS types and put @types in prod dependencies, I'd be interested to hear about them. Packages seem to be all over the place in how they handle the issue of transitive @types deps.

@webpro
Copy link
Collaborator

webpro commented Oct 11, 2023

@alecmev you can often take advantage of duck typing to sever @types dependencies, i.e. define an interface with just your needs in your public API that's compatible with the other package's type. Just doing my part to fight dependency bloat :)

@danvk Do you know of examples or ways to basically replace the transitive type deps with a dummy? Just to avoid the Cannot find module 'pkg' error. I'm thinking just any or declare module 'pkg' could do in my case: #283. Still not sure whether that use case is valid to begin with. Interested to hear your thoughts :)

@danvk
Copy link

danvk commented Oct 11, 2023

@danvk Do you know of examples or ways to basically replace the transitive type deps with a dummy? Just to avoid the Cannot find module 'pkg' error. I'm thinking just any or declare module 'pkg' could do in my case: #283. Still not sure whether that use case is valid to begin with. Interested to hear your thoughts :)

What I meant was more replacing:

import {SomeType} from 'big-complicated-package';

export function foo(obj: SomeType) { ... }

with:

export interface SomeType {
  just: string;
  fields: number;
  you: boolean;
  careAbout: Date;
}

export function foo(obj: SomeType) { ... }

By inlining just the portion of the type you actually care about, you broaden your API and get to sever a dependency. (You should test that your version of the type is compatible with the canonical one in your tests, but then you're in devDependency land.)

For severing transitive dependencies without code changes, that's a messy topic and I have a few thoughts :)

  • A particularly egregious example is Avoid depending on antlr4ts types in @pgtyped/query adelsz/pgtyped#451, where a runtime-only dependency (antlr4ts) that has no impact on the type declarations still pulls in 100+ .d.ts files. I asked about this on Stack Overflow. You can use the paths property of your tsconfig.json to stub out declarations for a transitive dependency.
  • Bloomberg apparently has an internal tool called dts-shake that can handle this in a more systematic way, but they've never gotten around to open sourcing it.

@webpro
Copy link
Collaborator

webpro commented Oct 22, 2023

Thanks a lot for chiming in and the references, Dan!

For simple types replacing external types is fine, but I guess in many cases this isn't very ergonomic or sustainable.

Just stumbled upon https://github.com/timocov/dts-bundle-generator. Interesting project! Has a few issues that prevents me from using it for Knip yet, but I'll play with it some more and keep watching it. I guess as long as Knip isn't imported and used programmatically (but only used from CLI) I can get away with it.

@webpro
Copy link
Collaborator

webpro commented Mar 21, 2024

Since I haven't had any more reports about this being an issue, I'm going to close it. Separating dependencies and devDependencies is a good best practice in general, and good default baseline for Knip. Also, there are workarounds (e.g. inline, peer deps).

@webpro webpro closed this as completed Mar 21, 2024
@alecmev
Copy link
Author

alecmev commented Mar 21, 2024

Hmm, I'm not sure, none of the three options seem satisfactory/viable to me:

  1. Keeping type-only packages in devDependencies that are mentioned in dist is simply unsound. I'm conflicted about @types/*-like implicit globals too, but what about the @microsoft/tsdoc example in the repro?

  2. Putting @types/foo in peerDependencies doesn't really make sense if foo itself is in dependencies.

  3. Duck-typing can work for dependency injection, like in this PR: Drop DOM types jonbern/fetch-retry#88 But it's not a panacea. What if I'm making a wrapper library for, say, Lodash? The API surface is colossal.

@alecmev
Copy link
Author

alecmev commented Mar 21, 2024

Sorry for two messages, forgot to add:

Since I haven't had any more reports about this being an issue

My suspicion is that most of the time people get lucky and there's a fitting @types/* somewhere else in the dependency tree, and in the unlucky cases, most people never type-check with skipLibCheck: false, so they just live with any.

@webpro
Copy link
Collaborator

webpro commented Mar 21, 2024

Sorry for two messages, forgot to add:

No worries! All signals are welcome. Currently I'm just trying to keep some sanity in (the number of) open issues.

For this particular issue, the cost/impact does not warrant the outcome (for me; feel tree to take a stab at it). I should definitely add it to the list of known issues: https://knip.dev/reference/known-issues. Sorry I forgot about that.

@alecmev
Copy link
Author

alecmev commented Mar 21, 2024

the cost/impact does not warrant the outcome

I respect that 😉 I do agree that this is somewhat "pedantic" territory.

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