refactor: 💡 resolve component path redundancy and public API encapsulation#798
refactor: 💡 resolve component path redundancy and public API encapsulation#798
Conversation
vineethasok
left a comment
vineethasok
left a comment
There was a problem hiding this comment.
I'm ok with almost all the changes except converting the test and stories to index format
The reason we added the same name as the component is it easily navigate to the appropriate file and test/stories file.
I understand it is possible using the folder name but we might add subcomponents inside the same folder which might need stories or tests
Would love to know your thoughts
| import { SidebarCollapsibleTitleProps } from './SidebarCollapsibleTitle/SidebarCollapsibleTitle'; | ||
| export type { Menu, SplitButtonProps } from './SplitButton/SplitButton'; | ||
| export type { ToastProps } from './Toast/Toast'; | ||
| import { SidebarNavigationTitleProps } from './SidebarNavigationTitle'; |
There was a problem hiding this comment.
nit: Since we are making changes to this files
Maybe we can also add the type prefix to all the content which are imported here
There was a problem hiding this comment.
@vineethasok ok thank you. What is the "content" you are referring to?
| { | ||
| group: ['**/index', '**/index.ts', '**/index.tsx'], | ||
| message: | ||
| "Do not import from index files within the same component directory. Import directly from source files (e.g., './Button' instead of './index').", |
There was a problem hiding this comment.
@hoorayimhelping @serdec
We were following a pattern in our company not to use ./ or ../ but @ based importing right?
Just to confirm the pattern we follow in click-ui and other products matches the same standard for coding
There was a problem hiding this comment.
The task here is not to request that the user use relative paths, but to prevent imports from index files. Please check the goal of the PR "path redundancy".
Ok that's great! You'll be happy to know that the first pass on file architecture is done in separate PR, the first pass is awaiting approve here #832 The PR here is stricly about "resolve component path redundancy". |
@hoorayimhelping see LSP Direct to source See PR attached video demo "LSP Direct to source" |
📚 Storybook Preview Deployed✅ Preview URL: https://click-ivd20bs5z-clickhouse.vercel.app Built from commit: |
hoorayimhelping
left a comment
hoorayimhelping
left a comment
There was a problem hiding this comment.
I'm still quite against barrel files and I don't think we should be using them, even in a library. IMO, the downsides far outweigh the benefits. https://tkdodo.eu/blog/please-stop-using-barrel-files
…ts types chore: 🤖 generate exports, e.g. expose direct component imports and its types
punkbit
changed the title
4 participants
Why?
To resolve component path redundancy and allow public API encapsulation.
As work progressed on reducing import path verbosity, several deeper issues surfaced that were addressed as part of this PR. Component import statements previously required the component name twice, e.g. clickhouse/click-ui/components/EllipsisContent/EllipsisContent, which was unnecessary. Beyond that, the original version exposed internal implementation details, allowing consumers to directly access and depend on third-party APIs such as Radix UI components and types. This has led to applications incorrectly coupling themselves to these internals rather than the library's intended public API, a problem that now requires careful, incremental cleanup using @deprecated warnings.
While addressing the above, circular dependencies were discovered throughout the source code. These were not anticipated but were resolved as part of this PR, and new ESLint rules have been introduced to prevent them from reappearing as the library grows.
Finally, after #773 (distribute unbundled) was merged, which solved critical distribution size issues, could now confirm that tree-shaking works correctly under the revised conditions and both import strategies, e.g. top-package level and component-level.
API improvements
@clickhouse/click-ui/components/EllipsisContent/EllipsisContent🤢@deprecatedwarnings to remove them gradually! The PR addresses this by encapsulating these details, ensuring only the deliberate public API surface is accessible.Build output size improvements
The original production version of the Click UI library had a critical bundling issue, producing a build output of 1,216.21 kB with chunks exceeding the 500 kB threshold after minification.
To benchmark the improvements, a baseline Vite app without Click UI was measured at 193.30 kB. After integrating the updated PR version of Click UI, the results were as follows:
Importing a component via the main barrel file / public API produced a build output of 223.70 kB, an overhead of just ~30 kB over the baseline. Importing directly from the component-specific export path (e.g. @clickhouse/click-ui/Button) brought this down marginally further to 223.09 kB.
Both approaches represent a dramatic reduction from the original, with the PR version adding less than 30 kB over a bare Vite app regardless of import strategy.
This is made possible by several changes to resolve component paths and, of course, by the introduction of #773, which makes the package distribution unbundled and moves optimisation responsibility to the consumer side. Before, the consumer always had an unscalable bundled/unoptimizable 😬 package of 1,216.21 kB.
👇 Read below for supporting evidence
❌ The original production version (build output size 1,216.21 kB, plus chunks are larger than 500 kB after minification)
🔎 Created a base to demonstrate the performance improvements, e.g. a Vite app without Click UI (build output size 193.30KB)
The base, e.g. a Vite app shows a placeholder component (build output size 193.30KB)
👌 Vite app with Click UI PR Version imports a Click UI Component from main barrel file / Public API (build output size is 223.70 kB)
👌 Vite app with Click UI PR Version imports a Click UI Component directly from component path (build output size is 223.09 kB)
🤖 TODO: Once #773 is merged, switch base branch to main.
How?
import/no-cycle, and prevent import from barrel filesPreview?
Distribution (to keep it short shows a small example for ESM Button), if you wante more see #773
LSP direct to source or goto implementation is unaffected, e.g. you don't land in barrel but directly in the source code
Modal/terminal-based text editors
demo-lsp-editor-direct-source.mov
editor-goto-identifier.mov
VSCode-like text editors
demo-goto-source-vscode.mov
ESLint rules
Warns against importing from the main barrel within the library itself, guiding contributors towards importing directly from leaf modules instead. Also, component-level index files are reserved for cross-component imports. These rules help prevent circular dependencies from forming.
Benchmarks
To address any concerns around the use of module re-exports and show responsible and safe use of barrel files, benchmark results are available below:
NOTE: Have omitted markers to keep it short. The benchmark file can be run in your local environment and modified to your liking. Find them at here