webpro-nl / knip

✂️ Find unused files, dependencies and exports in your JavaScript and TypeScript projects. Knip it before you ship it!
https://knip.dev
ISC License
6.97k stars 173 forks source link

🆕 New plugin API (request for feedback) #540

Closed webpro closed 8 months ago

webpro commented 8 months ago

Today, Knip has 55 plugins, and they're all using a somewhat complicated findDependencies function. Merging quite a few plugins, and seeing various requirements and implementations, gave me a better understanding of what we need here and how the plugin API could be improved.

So that's what I'm trying to do: gather your feedback! Just a thumbs up 👍 is great, too :)

New to Knip plugins? 🦄

If you're new to Knip plugins, perhaps it's best to ignore any history here and see whether the updated docs make sense: Writing A Plugin. Then you might want to skip straight to WDYT.

Implemented or fixed a plugin before? 😎

If you're part of Knip's elite Cool Club™️ and implemented or fixed a plugin before, here are the subtle improvements:

Yet the major difference is in what's currently the findDependencies function. Now there's just 3 functions to mix & match:

On one hand it might feel hard to justify going from 1 function to 3. On the other hand, plugin implementations are arguably easier to understand and need less code. Here's a few side-by-side diffs:

1008 insertions(+), 1585 deletions(-)

No breaking changes

There are no breaking changes, since no user config needs to change, and everything is still internal to Knip. Plugin API is not (yet?) exposed for external/custom plugins.

Try it before the release

The unit tests are passing, the integration suite too. But nothing beats actual coverage on your own projects! Would be great if you could run this version of Knip on your project(s):

npm install knip@plugins

Please report any regressions you may find.

WDYT?

name desc
resolveConfig Receive config object, extract & return dependencies
resolveEntryPaths Receive config object, extract & return entry paths
resolve No config object, none of the above, do anything

Nothing is set in stone, no rush. If you have ideas or know about similar examples (that have different APIs) I'm all ears! Thanks for reading 🙏

webpro commented 8 months ago

cc-ing a few people recently active in this area (hope you don't mind!) @beaussan @uncenter @jgoux @drodil @risu729 @jmcpeak @thenbe @what1s1ove @voxpelli

uncenter commented 8 months ago

No problem on the ping! The changes look clean and helpful, and I've done some testing on a couple of repositories and can report zero regressions for the Eleventy and Netlify plugins.

Overall, the new function and export naming is definitely more understandable - moving from SCREAMING_SNAKE_CASE to camelCase really helped!

webpro commented 8 months ago

Thanks @uncenter! Great to hear. Appreciate you've run it on those projects, this builds up confidence in the changes.

I forgot, the clearest view on the changes (next to the docs) is probably the new template: https://github.com/webpro/knip/blob/plugins/packages/knip/src/plugins/_template/index.ts

ryanwilsonperkin commented 8 months ago

Plugin API is not (yet?) exposed for external/custom plugins.

Would this be a good time to start opening knip up to external/custom plugins? I'm working on a large application that has a need for custom plugins that are specific to the application and wouldn't really make sense as an internal feature of knip. It would be great to be able to do something like:

// knip.config.ts
import MyCustomPlugin from './my-custom-plugin';

const config: KnipConfig = {
  plugins: [MyCustomPlugin],
}

export default config;
beaussan commented 8 months ago

No worries for the ping!

In general, do you think the plugin API makes sense? Is something still missing?

First of all, I know this comes down to personal preferences so feel free to reject my idea.

I would love to expose a single object that can be strongly typed instead of relying on magic exports based on their names. This means that discoverability of api as a plugin is easier since they are automatically exposed via type completion, and it's a safer way to write a plugin given it's type safe.

There might be a reason this was not the case however that I might be missing 👀

webpro commented 8 months ago

Would this be a good time to start opening knip up to external/custom plugins?

My own proposal just got merged and released earlier today. Let's see how this goes. To be honest, at this point I'm a bit on the fence with the idea, since it'll obviously also increase API surface and maintenance burden.

webpro commented 8 months ago

No worries for the ping!

In general, do you think the plugin API makes sense? Is something still missing?

First of all, I know this comes down to personal preferences so feel free to reject my idea.

I would love to expose a single object that can be strongly typed instead of relying on magic exports based on their names. This means that discoverability of api as a plugin is easier since they are automatically exposed via type completion, and it's a safer way to write a plugin given it's type safe.

There might be a reason this was not the case however that I might be missing 👀

The plugins indeed have a single default-exported object. Just merged it earlier today :)

Like so? https://github.com/webpro/knip/blob/main/packages/knip/src/plugins/_template/index.ts#L33-L34

Realized not all plugins did this yet, so that's fixed up too now. Either way, since everything is internal to Knip we could still change anything :)

beaussan commented 8 months ago

Exactly like so! I've must have looked at some plugins that didn't had their migration yet

webpro commented 8 months ago

Closing this one as it's merged. Thanks for the help & feedback!