Skip to content

All packages: Remove unused dependencies #31227

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

Merged
merged 47 commits into from
Apr 28, 2025

Conversation

webpro
Copy link
Contributor

@webpro webpro commented Apr 25, 2025

Closes #

As a follow-up to discussion in e18e Discord this is a start to remove unused dependencies. We can improve later on and focus more on files and exports, but this one's pretty straightforward using knip --dependencies --fix

What I did

knip --dependencies --fix

And a few corrections for false positives.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

This PR performs a comprehensive cleanup of unused dependencies across multiple Storybook packages using the knip tool, improving maintenance and reducing package sizes.

  • Removed key UI dependencies from core package including @popperjs/core, react-draggable, and markdown-to-jsx
  • Added missing dependencies @babel/core and @types/babel__core to addon-docs package
  • Removed React-related dependencies from Angular framework (@types/react, @types/react-dom)
  • Removed Vue compiler dependency @vue/compiler-core from Vue3 renderer
  • Removed accessibility polyfill resize-observer-polyfill from a11y addon

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

Copy link

nx-cloud bot commented Apr 25, 2025

View your CI Pipeline Execution ↗ for commit f8812e6.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 1m 40s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-28 12:23:49 UTC

@webpro
Copy link
Contributor Author

webpro commented Apr 25, 2025

@yannbf
Copy link
Member

yannbf commented Apr 28, 2025

Could this be a flaky test? app.circleci.com/pipelines/github/storybookjs/storybook/96719/workflows/32bd7050-e23f-44dd-89ac-4fbc7bd04a5e/jobs/813424

yes, it has nothing to do with your changes

… @adobe/css-tools, @angular-devkit/architect, and webpack to their latest versions for improved performance and security. Remove outdated entries and ensure compatibility with newer versions of TypeScript and Angular.
…sions for improved dependency management and compatibility.
…or improved type safety. Update Next.js app-router-provider to use eslint-disable comments for TypeScript ignore directives, ensuring compatibility with Next.js v15.1.1 and above.
@ndelangen ndelangen merged commit bbf1413 into storybookjs:next Apr 28, 2025
90 checks passed
@github-actions github-actions bot mentioned this pull request Apr 28, 2025
13 tasks
@webpro
Copy link
Contributor Author

webpro commented Apr 28, 2025

Good stuff y'all! 🙌

Noticed that we can probably get rid of the custom package.json#bundler logic to assemble Knip config, now that Knip supports package.json#exports and also maps those to source files—which largely seems to match the bundler entries.

@webpro webpro deleted the chore/remove-unused-dependencies branch April 28, 2025 13:02
@JReinhold
Copy link
Contributor

Thanks for your help @webpro !

@@ -55,8 +55,6 @@
"prep": "jiti ../../../scripts/prepare/bundle.ts"
},
"dependencies": {
"@storybook/global": "^5.0.0",
"ts-dedent": "^2.0.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have been wrong, after updating to beta.5, I'm getting "Could not resolve "ts-dedent" imported at @storybook/svelte/dist/components/PreviewRender.svelte:8:23.

Copy link

@lishaduck lishaduck Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the code:

It seems knip's svelte compiler wasn't working?
Presumably this wasn't caught in tests because the repo has other usage of ts-dedent, so it was hoisted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The svelte compiler is working, but the issue is no less than four-fold:

  • the source file src/components/PreviewRender.svelte isn't in package.json#bundler.entries
  • it's in package.json#exports but the dist folder is git-ignored so the file isn't part of the analysis in Knip to get imports/exports from
  • the package.json#exports entry should've been "source-mapped" from dist/components/PreviewRender.svelte to src/components/PreviewRender.svelte, however that doesn't happen because of the non-standard file extension → this is something I'll fix in Knip
  • but even when that's fixed, the source mapper isn't working since outDir isn't specified in tsconfig.json (it's built using something that has outDir defined elsewhere)

So in this case I think we'll have to put it back manually, Knip can't detect it properly (at this point it should report it under "Unlisted dependencies"). Unless we're adding outDir: "dist" to tsconfig.json.

Copy link
Contributor

@JReinhold JReinhold Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @webpro!

the source mapper isn't working since outDir isn't specified in tsconfig.json (it's built using something that has outDir defined elsewhere)

Can you tell me more about this? Why is does Knip need to map to sources, why doesn't it just look directly in the dist that package.json#exports is pointing to? I understand that because dist is gitignored, we would actually have had to build all pacakges before running Knip.

Unless we're adding outDir: "dist" to tsconfig.json.

I'm fine with doing this for the Svelte package, because our build scripts ignores that property, but it's actually misleading. Because we use a bundler, there's no one-to-one mapping between dist and source files, like there would have been if we just used tsc, that just copies the file-structure 1-to-1 AFAIK. This only works for our .svelte-files because we actually do copy those 1-to-1 without bundling. But in all other cases, Knip would be misled, right?


Is there perhaps an alternative solution, where we modify the Knip config for the Svelte packages/files, to let it know of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @webpro!

the source mapper isn't working since outDir isn't specified in tsconfig.json (it's built using something that has outDir defined elsewhere)

Can you tell me more about this? Why is does Knip need to map to sources, why doesn't it just look directly in the dist that package.json#exports is pointing to? I understand that because dist is gitignored, we would actually have had to build all pacakges before running Knip.

Only the actual source files are reliable. Dist files could be bundled/concatenated (as you explain below) and sometimes have import specifiers that can't be resolved. This could e.g. lead to false positives and dependencies/exports attached to the wrong files. I should expand on this more and provide examples at https://knip.dev/features/source-mapping.

And yes, at this point dist files need to be produced before Knip can do the mapping. There might be cases where this build step isn't necessary, but that's where we are today.

Unless we're adding outDir: "dist" to tsconfig.json.

I'm fine with doing this for the Svelte package, because our build scripts ignores that property, but it's actually misleading. Because we use a bundler, there's no one-to-one mapping between dist and source files, like there would have been if we just used tsc, that just copies the file-structure 1-to-1 AFAIK. This only works for our .svelte-files because we actually do copy those 1-to-1 without bundling. But in all other cases, Knip would be misled, right?

If there's not only compilation but also bundling then it's a different story indeed. Doesn't feel right to do this just for Knip if it's not actually used otherwise. A quick glance at the scripts/prepare folder shows it might be something to consider for the future (i.e. to use compilerOptions.outDir)?

Is there perhaps an alternative solution, where we modify the Knip config for the Svelte packages/files, to let it know of this?

Shouldn't it be added to package.json#bundler.entries? It is an entry file after all?

Otherwise, by exception, we can just add it as an entry file to the Knip config. I'll whip up a PR for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you already did that, thanks Jeppe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:daily Run the CI jobs that normally run in the daily job. dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants