search

TanStack / table

🤖 Headless UI for building powerful tables & datagrids for TS/JS - React-Table, Vue-Table, Solid-Table, Svelte-Table
https://tanstack.com/table
MIT License
24.61k stars 3.04k forks source link

fix: properly mark type of arrays that are not modified by the librar… #5452

Closed pcorpet closed 3 months ago

pcorpet commented 4 months ago

…y as readonly

Fixes https://github.com/TanStack/table/discussions/3648

This is a new attempt after #4909 was closed.

KevinVandy commented 4 months ago

I don't really understand why people want readonly added to the TanStack Table types. Is it necessary? Nice to have? I guess can see the case for nicely integrating data that was already marked as readonly from some state managers or graphql clients without needing to cast, but I don't understand the advantage for all of these other changes.

pcorpet commented 4 months ago

Most of the time when handing over an array you don't expect the function or the component that takes that array to be able to modify them. So you're actually thinking of a readonly array. One could think that it should be the default for all arrays in most code with the mutable arrays being the exception.

Here are two use cases that benefit of readonly array in addition to state managers and graphql clients:

  1. Using as constto strong type quickly. In our code we regularly write static const values with the as const, this helps us restrict the type of the constant to its bare minimum. However this creates readonly arrays.

  2. Stabilizing props: in order to improve performance, we recommend our developers to treat data as immutable. See this article.

If my proposal is a no-go for you, I can try to restrict to the bare minimum and restrict only the interface of TanStack's library.

nx-cloud[bot] commented 4 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b89f67b379280a1aeb187a55ae4cfdbd1e30a8bb. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

🟥 Failed Commands
nx affected --targets=test:format,test:sherif,test:knip,test:lib,test:types,build --parallel=3

Sent with 💌 from NxCloud.

KevinVandy commented 4 months ago

The builds are not passing after these changes