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

Add Knip with initial config #20572

Closed
wants to merge 7 commits into from
Closed

Add Knip with initial config #20572

wants to merge 7 commits into from

Conversation

webpro
Copy link
Contributor

@webpro webpro commented Jan 11, 2023

Knip is a tool to find unused files, dependencies and exports. There are multiple ways to use it, and in the Storybook repo, for starters, it could be a great companion for clean ups and refactorings.

This PR contains Knip with some initial configuration to get started. Analysis can be done for the project as a whole, or per workspace. Also, output can be filtered for specific issue types. A few example commands to get an idea:

yarn knip
yarn knip --workspace ui/manager --include dependencies,unlisted
yarn knip --workspace ui/manager --include files

Please refer to the Knip docs for more details.

The Storybook project would benefit from a programmatic API and/or dynamic configuration (e.g. knip.ts instead of knip.json). For instance, to read the entry files from the package.json#bundler.entries in many workspaces and merge into the Knip config. Both are on Knip's roadmap.

Please let me know if you have any questions or feedback regarding Knip or this PR specifically.

Let's get this collaboration started and find out how we can help each other while improving DX for everyone :)

@ndelangen ndelangen self-assigned this Jan 14, 2023
@ndelangen ndelangen added the build Internal-facing build tooling & test updates label Jan 14, 2023
@ndelangen
Copy link
Member

@webpro thanks for bringing this my my attention, and thank you for the detailed explanation of knip in the meeting.

I don't see knip being used anywhere? no CI config change, and also no documentation on how to run things manually.

We chatted about integrating it into the check script that each of our packages have.

How do you propose we move this PR forwards?
I'm trying to lower the PR count on storybook, so it be nice if we have some plan for eventually getting this PR moving towards a goal.

@ndelangen ndelangen self-requested a review January 14, 2023 10:23
@webpro
Copy link
Contributor Author

webpro commented Jan 16, 2023

I don't see knip being used anywhere? no CI config change, and also no documentation on how to run things manually.
How do you propose we move this PR forwards?

That's what we still need to discuss here. Gave a few example commands to run it manually and get a feel for what Knip does (docs: https://github.com/webpro/knip). Depending on the project needs and goals there are a few ways to go about getting this PR merged.

Since I think most (if not all) of the workspaces/packages are not without issues according to Knip, I would advice to decide on some focus areas first and then gradually work towards a situation which the maintainers are happy about.

Questions:

  • Which (type of) workspaces do you want to start out with?
  • Do you prefer to focus first on --dependencies, or --exports? Or both, but in that case I'd advice starting out with less workspaces.
  • Which CI workflow should this be added to, in a new or existing job? I see you're using CircleCI (not familiar, but should be easy).

Notes:

  • The workspaces have varying entry points.
    • I'm not familiar enough with the codebase to decide/automate to put this into the configuration. So this needs to be reviewed.
    • We also chatted about adding package.json#bundler.entries into Knip's configuration automatically. I've used the latest version of Knip supporting dynamic configurations, and also added this automation to this PR.
  • Since fixing issues is out of scope for this PR, we will probably need some ignores/filters and maybe --no-exit-code until the issues are fixed.

We chatted about integrating it into the check script that each of our packages have.

This is possible indeed. It requires a knip config in each of those packages. Added an example just now in the ui/managerpackage.

@webpro
Copy link
Contributor Author

webpro commented Jan 16, 2023

Oh I forgot to mention: when running knip from the ui/manager package directory, the last line of the root code/knip.ts needs to be commented out:

// export default addBundlerEntries(baseConfig);

Quickly got into TS config issues when separating this and didn't want to introduce more changes into this PR at this point.

@ndelangen
Copy link
Member

@webpro eventually I'd like to move the scripts to run out of the code directory, and into the scripts directory. To keep dependencies separate.

@ndelangen
Copy link
Member

@webpro I'll keep knip in mind, and will come back to it later.

Thank you for your time, energy and expertise working on this PR.
Sadly I'll close it for now.

@ndelangen ndelangen closed this Nov 24, 2023
@webpro webpro deleted the feature/introduce-knip branch November 24, 2023 16:06
@webpro
Copy link
Contributor Author

webpro commented Nov 24, 2023

Of course, Norbert. Let me know if/when I can help. Best of luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants