Skip to content

[charts] Use knip to remove dead code#21887

Open
alexfauquette wants to merge 10 commits intomui:masterfrom
alexfauquette:knip
Open

[charts] Use knip to remove dead code#21887
alexfauquette wants to merge 10 commits intomui:masterfrom
alexfauquette:knip

Conversation

@alexfauquette
Copy link
Copy Markdown
Member

@alexfauquette alexfauquette commented Mar 27, 2026

Experimenting with knit.dev

Usecase per commit

  • classes item: Most of the classes files export the get...UtilityClasses() and Use...ClassesOptions that are helpers to make the code safer and easier to read. But those have no impact on users, and are never used in other files.
  • dead files: dead files we forgot to remove during large PR
  • styled element: Small component created to pass some default CSS
  • dead code: some functions, const, or types that we defined for latter usage, or we forgot to remove them during a refactoring.
  • locale types: Most of those types are:
    • props of internal components. They are defined for type checking but we build nothing on top of those interface
    • attribute of internal hooks/helpers
  • locale helpers: Some const object used to define default values, helpers to simplify computation, or intermediate selectors.
  • export errors: The most interesting part, those const/type got identified by knip as unused, but for consistency with the rest of the package, they should be public exports. So I'm fixing the export

Impacts

Bundle reduction

The gain in terms of bundle is negligeable. It removed few lines, or export from generated files
https://frontend-public.mui.com/diff-package?package1=https%3A%2F%2Fpkg.pr.new%2Fmui%2Fmui-x%2F%40mui%2Fx-charts-pro%403fede28&package2=https%3A%2F%2Fpkg.pr.new%2Fmui%2Fmui-x%2F%40mui%2Fx-charts-pro%407b36606#file-FunnelChart-funnelClasses-d-ts

DevEx

This prevent use from exporting everything, which makes it easier to spot missing exports when reading a file.

The docs:api that generate the list of exported elements make it easy during a PR to spot exported elements that should not be exported. This should simplify the oposit: spotting elements that are not exported but should be. A line interfacte UseXxxParams without an export is easier to spot than a missing export * from ./useXxx.ts in the index file

@Janpot what's your thought about adding that to the CI?

@mui-bot
Copy link
Copy Markdown

mui-bot commented Mar 27, 2026

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running pnpm l10n
  • Clean files with pnpm prettier.

Deploy preview: https://deploy-preview-21887--material-ui-x.netlify.app/

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts ▼-45B(-0.01%) ▼-25B(-0.02%)
@mui/x-charts-pro ▼-186B(-0.04%) ▼-61B(-0.04%)
@mui/x-charts-premium ▼-61B(-0.01%) ▼-37B(-0.02%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 87b4d87

@alexfauquette alexfauquette added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. scope: charts Changes related to the charts. scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). labels Mar 30, 2026
@alexfauquette alexfauquette marked this pull request as ready for review March 30, 2026 08:50
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 30, 2026

Merging this PR will improve performance by 21.99%

⚡ 1 improved benchmark
✅ 13 untouched benchmarks

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation ScatterChartPro with big data amount and zoomed in (single renderer) 94.2 ms 77.2 ms +21.99%

Comparing alexfauquette:knip (87b4d87) with master (b322127)1

Open in CodSpeed

Footnotes

  1. No successful run was found on master (dffb2c6) during the generation of this report, so b322127 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Janpot
Copy link
Copy Markdown
Member

Janpot commented Mar 30, 2026

some type export that are not used. For example XxxParams fro an internal hook is not necessary

I don't get this. Isn't it good form to export types that are used by your other exports? Even for an internal hook, consumers of these hooks may want to type their code without having to resort to ReturnType<typeof useMyHook>. Doesn't mean you have to export it as a public export of the package.

@JCQuintas
Copy link
Copy Markdown
Member

We should probably split this pr, there are a lot of different contexts where I have to know "why was this changed?"

@alexfauquette
Copy link
Copy Markdown
Member Author

alexfauquette commented Mar 30, 2026

I don't get this. Isn't it good form to export types that are used by your other exports? Even for an internal hook

IMO either the hook is publicly export, and so we should export it's type. Or it's an internal one, and so we should export it only if the type is used.

This has the benefit of identifying what is not exported. Currently we export nearly everything. So at the end of the day we don't know which exports are used, and which one are just due to habits, and do not bubble up to the public interface.

By removing unused export we constraint the usage of export and so increase the probability to catch missing exports.

I've reorganised commit to give them some meaning. Detailed description are in the PR description :)

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged. and removed PR: out-of-date The pull request has merge conflicts and can't be merged. labels Mar 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: out-of-date The pull request has merge conflicts and can't be merged. scope: charts Changes related to the charts. scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants