-
Notifications
You must be signed in to change notification settings - Fork 528
Description
Describe the bug
Installing the data table component via the Dice UI website causes an unpredictible installation with warnings, errors, incorrect imports and disparity with the code present in this repository. Aditionally, some architectural concerns I noticed may be of help.
How to reproduce
Unpredictable installation
Executing:
npx [email protected] add "https://diceui.com/r/data-table"
Fails:
npx [email protected] add "https://diceui.com/r/data-table"
An invalid components.json file was found at C:\Users\S86OWUJ\IdeaProjects\timecop\src\main\client-experiment.
Before you can add components, you must create a valid components.json file by running the init command.
Learn more at https://ui.shadcn.com/docs/components-json.
This is most likely because of the version of shadcn used: "2.4.0-canary.12". There is no documented reason, at least to my knowledge, behind the usage of this version.
Switching to "latest" resolves the aforementioned issue, but causes the ones originally described in this issue.
npx shadcn@latest add "https://diceui.com/r/data-table"
The problems are contained in the following files, though, in general, files seem to be outdated if compared with the current contents of this repository.
- The file
components/data-table/data-table.tsxcontains, at line 14, the following import:import { getCommonPinningStyles } from "@/components/data-table/data-table";, which is circular. - The file
components/data-table/data-table-faceted-filter.tsxcontains, at line 25, the following import:import type { Option } from "@/components/data-table/data-table";, which is simply incorrect, given thatdata-table.tsxdoesn't contain an exported member namedOption. - The hook
hooks/use-data-table.tscontains an import at line 34:import type { ExtendedColumnSort, QueryKeys } from "@/components/data-table/data-table";that is also incorrect. types/data-table.tsalso contains an incorrect import:import type { DataTableConfig } from "@/components/data-table/data-table";.
Note: I'm noticing lots of standard ESLint and SonarQube warnings present in the files too, even the updated ones copied from this repository. This is not inherently wrong, but sometimes, it is standard practice to ensure code passes multiple linters before deploying. More so when it comes to reusable components like these, where the code will flag developer linters.
These problems are not present in the current repository; copying and pasting the code from this repo to my own project appears to resolve them, so the errors documented above are just meant to illustrate the amount of problems that I (and probably others) are facing by just trying to follow the Dice UI docs.
Additionally, there is a maintainability concern here: we ought to trust the installation command for upgrades too, this makes me and our team insecure.
Architectural concerns
After taking a look at the component provided, the files and the directory structure that was chosen, I have some comments that the team behind tablecn might appreciate, though these can be taken with a grain of salt, given that guidelines differ between projects and developers, regardless:
Placement of hooks and types
The hooks and type files should be placed within the data-table/ folder because their usage is contained exclusively within the data table component. This follows the SRP, where each module should have a clear, cohesive purpose. By colocating related files, we improve discoverability and maintainability, developers can immediately understand the scope and dependencies of the data table without hunting through generic hooks/ and types/ directories.
This also reduces coupling with the rest of the application, as the data table becomes a self-contained module with clear boundaries. When components own their dependencies, it's easier to refactor, test, or even extract them into separate packages without breaking unrelated parts of the codebase.
Hardcoded strings
Hardcoding language strings is not ideal for reusable components, as it limits internationalization and customization. User-facing text like labels, placeholders, and error messages should be configurable through either the component config or, preferably, through props. Modifying the source is always an option, but it means the component is not fully reusable as-is.
Link to reproduction
Reproduction is redundant here
Additional information
No response