knip icon indicating copy to clipboard operation
knip copied to clipboard

🧩 Complex plugin challenge

Open BryanCrotazGivEnergy opened this issue 11 months ago • 11 comments

Discuss anything related to Knip

I'm writing an sst plugin. Most of it's fine, but sst builds locally before deploying, and represents entry point files as strings. Is there any ast type parsing built into knip that would be able to traverse a code tree to find what an interface string property was set to?

BryanCrotazGivEnergy avatar Mar 10 '25 11:03 BryanCrotazGivEnergy

Great question, and unfortunately there are no affordances to this end yet. It's becoming more and more clear that we need this type of static analysis in plugins too.

The APIs for that aren't clear yet. Technically you could already import ts from 'typescript' in any plugin and use it inside e.g. resolveConfig as we have the path the config file available. My first thought is still that the TypeScript AST + APIs are a bit much, perhaps we could do with easier interfaces. In the end it is, greatly simplified, mostly a matter of extracting strings out of the source file. If you have any ideas or suggestion here they'd be most welcome!

Also see https://github.com/webpro-nl/knip/pull/856 where something similar is initiated/discussed.

sst builds locally before deploying

Out of curiosity, what do you mean by this exactly? Got an example you could link to maybe?

webpro avatar Mar 10 '25 11:03 webpro

Out of curiosity, what do you mean by this exactly? Got an example you could link to maybe?

SST builds serverless systems. If you have two lambdas, you have to build two compiled js files. SST builds them separately, so there are two extra entry points, but they're not referenced by an import anywhere.

BryanCrotazGivEnergy avatar Mar 11 '25 13:03 BryanCrotazGivEnergy

A snippet of my plugin:

import { join } from 'path';
import type { IsPluginEnabled, Plugin, ResolveConfig, ResolveEntryPaths } from '../../types/config.js';
import { toDeferResolve, toEntry, type Input } from '../../util/input.js';
import { hasDependency } from '../../util/plugin.js';
import type { PluginConfig } from './types.js';

// link to sst docs

const title = 'sst';

const enablers = ['sst'];

const isEnabled: IsPluginEnabled = ({ dependencies }) => hasDependency(dependencies, enablers);

const config: string[] = [];

const entry = ['sst.config.{js,cjs,mjs,ts}'];

const production: string[] = [];

const resolveConfig: ResolveConfig<PluginConfig> = async config => {
  console.log('resolveConfig config', config);
  const inputs = config?.plugins ?? [];
  return [...inputs].map(toDeferResolve);
};

const resolveEntryPaths: ResolveEntryPaths<ViteConfigOrFn | VitestWorkspaceConfig> = async (
  localConfig,
  options
) => {
  console.log('resolveEntryPaths localConfig', localConfig);
  console.log('resolveEntryPaths options', options);
  const inputs = new Set<Input>();
  return Array.from(inputs);
};

export default {
  title,
  enablers,
  isEnabled,
  entry,
  resolveEntryPaths,
  resolveConfig
} satisfies Plugin;

The resolveEntryPaths console logs are not appearing in the test - do you know why?

Is there a function that gets called on each typescript file that's checked for dependencies? My entry point string could be in a file that's imported from the config.

BryanCrotazGivEnergy avatar Mar 11 '25 13:03 BryanCrotazGivEnergy

Out of curiosity, what do you mean by this exactly? Got an example you could link to maybe?

SST builds serverless systems. If you have two lambdas, you have to build two compiled js files. SST builds them separately, so there are two extra entry points, but they're not referenced by an import anywhere.

These paths aren't known upfront cq can't be derived from loading the config file?

If there are default target paths for those files, they can be added as entry points in the plugin. If SST users override such defaults in SST config, ideally we can read that out in the Knip plugin from the SST config file, but this isn't strictly necessary (then Knip users need to override the paths in sst.entry as well).

The resolveEntryPaths console logs are not appearing in the test - do you know why?

Is there a function that gets called on each typescript file that's checked for dependencies? My entry point string could be in a file that's imported from the config.

Not sure, but here's where this call happens in the Knip implementation:

https://github.com/webpro-nl/knip/blob/54834256ab7ed293ccf26068cdbcab431f35d4e3/packages/knip/src/WorkspaceWorker.ts#L353-L357

Maybe config is undefined when loading e.g. sst.config.ts? Knip uses its default export.

webpro avatar Mar 11 '25 13:03 webpro

SST builds serverless systems. If you have two lambdas, you have to build two compiled js files.

Note that Knip is eager to analyze source files, not build artifacts. I haven't worked with SST myself, but can't remember having come across a good reason to make an exception to this rule yet.

webpro avatar Mar 11 '25 13:03 webpro

In SST you call an SST helper passing in a descriptor about your lambda to be built, where the path to your entry point is a string property of the hash:

My fixture example:

import { d } from "dependencyFromStack";
import { use, StackContext, Function, FunctionProps } from "sst/constructs";

export function UserDataServiceLambdaStack({ stack, app }: StackContext) {

  // Create single Lambda handler
  const handlerProps: FunctionProps = {
    handler: "src/handlers/my-handler.handlerFn",
    permissions: ["perm1", "perm2"]
  };

  const handler = new Function(stack, "MyHandler", handlerProps);

  return {
    handler
  };
}

I'm trying to pull out src/handlers/my-handler.ts as the new knip entry path - the .handlerFn is the name of the function inside that file.

The above file is imported by the config file.

BryanCrotazGivEnergy avatar Mar 11 '25 13:03 BryanCrotazGivEnergy

Note that Knip is eager to analyze source files, not build artifacts. I haven't worked with SST myself, but can't remember having come across a good reason to make an exception to this rule yet.

This is no exception - the useful information is in the src files.

BryanCrotazGivEnergy avatar Mar 11 '25 13:03 BryanCrotazGivEnergy

Gotcha. That's like two steps further than what Knip currently has affordances for. Like, not only the AST of sst.config.ts, but also of an import it does, right?

I'm afraid adding entry: ["src/handlers/*.ts"] for starters is too custom/naive/simplistic? Or some default like that. Plugin could still be very useful, but users need to be aware that overrides/differences here need to be mirrored in their Knip config as well (that's documented in Knip docs and we could add extra notes on SST plugin page).

webpro avatar Mar 11 '25 14:03 webpro

users need to be aware that overrides/differences here need to be mirrored in their Knip config

That's what I'm trying to avoid - the reason I chose to use knip is to find developers' mistakes. We're moving from npm to pnpm and we've discovered a ton of missing dependencies in the 75 project monorepo than npm let us get away with. Finding knip was fabulous - immediately discovers most of our issues, but doesn't know about the sst entry points.

So the config is ASTed? Could that be done recursively with an optional plugin-supplied callback for other non-config files? You must already be recursing for dependency discovery?

BryanCrotazGivEnergy avatar Mar 11 '25 15:03 BryanCrotazGivEnergy

I'm happy to submit a PR if this would be a good way of doing it, but you'd need to tell me where to look for the recursive ts file processing

BryanCrotazGivEnergy avatar Mar 11 '25 15:03 BryanCrotazGivEnergy

users need to be aware that overrides/differences here need to be mirrored in their Knip config

That's what I'm trying to avoid

Yes, I gathered the plugin would still be useful for the plugins dependencies (and maybe more?). But not having the handler entry paths would make it less interesting.

  • the reason I chose to use knip is to find developers' mistakes. We're moving from npm to pnpm and we've discovered a ton of missing dependencies in the 75 project monorepo than npm let us get away with. Finding knip was fabulous - immediately discovers most of our issues, but doesn't know about the sst entry points.

So the config is ASTed? Could that be done recursively with an optional plugin-supplied callback for other non-config files? You must already be recursing for dependency discovery?

Yes, all entry files are part of the TS program and their AST is traversed by Knip's core. But this isn't available for plugins. At least not yet.

I'm happy to submit a PR if this would be a good way of doing it, but you'd need to tell me where to look for the recursive ts file processing

Not sure yet what API(s) to introduce for this. There are a few options here, I can think of a few ways to go about it:

  • Add a function that plugins can implement (much like resolveConfig) and send the file path or AST (instead of the localConfig object). Downside: probably requires to load/parse files twice.
  • Add so-called visitor functions that plugins can implement and Knip can add/use during existing AST traversal, and somehow route the results back to the plugin. Downside: added complexity.

We'd have to think through possible use cases a but more before making a decision. E.g. there's also use cases like #848.

Without having to waiting for any of this, what you could consider for now: use a dynamic knip.ts and do the parsing of config files etc. manually and add entry points to the Knip entry: [] config. For the rest, if you want, we could start out with a simple SST plugin that does not do this AST part yet but does handle the plugins and perhaps more properties like that if there are any.

webpro avatar Mar 12 '25 06:03 webpro

I've submitted a crude v1 plugin PR that uses globs. Can we get this merged and published to npm please?

BryanCrotazGivEnergy avatar Mar 13 '25 10:03 BryanCrotazGivEnergy

Closing this as #1005 has been merged.

webpro avatar Apr 08 '25 07:04 webpro