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

Feat: qwik-table adapter #5420

Closed anxhirr closed 4 months ago

anxhirr commented 4 months ago

Hi TanStack team,

I've created a qwik-table adapter designed to integrate the table core with the Qwik framework. I have tested(not fully) and created examples for column sorting, pagination, and row selection.

I thought it was a good idea to share this as a first version and get some thoughts on whether this looks promising or not.

I welcome any feedback and look forward to contributing/helping make the adapter as perfect as the core itself is xD.

This discussion is related: https://github.com/TanStack/table/discussions/5281

KevinVandy commented 4 months ago

Looks good, will probably test this out tomorrow!

anxhirr commented 4 months ago

Looks good, will probably test this out tomorrow!

Cool, In the meantime I'll be testing and adding more examples as soon as I find free time.

nx-cloud[bot] commented 4 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 83c159b941418c50818340ef4e12e75a9c00be51. 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


✅ Successfully ran 2 targets - [`nx affected --targets=test:format,test:sherif,test:knip,test:lib,test:types,build --parallel=3`](https://cloud.nx.app/runs/H9N0v3Nsjx?utm_source=pull-request&utm_medium=comment) - [`nx run-many --targets=test:format,test:sherif,test:knip,test:lib,test:types,build --parallel=3`](https://cloud.nx.app/runs/exh72g6OAy?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

KevinVandy commented 4 months ago

@anxhirr I worked a bit on this PR. I think it might be in a better place now. All tests passing. Build output looks fine.

What's the dummy state in the adapter for though?

anxhirr commented 4 months ago

@anxhirr I worked a bit on this PR. I think it might be in a better place now. All tests passing. Build output looks fine.

What's the dummy state in the adapter for though?

On the first version, I was getting some serialization error if I did not provide it. Now, That error seems to be gone so we can simply remove it

EDIT: I just double-checked and yes there is still an error if pageSize, pageIndex, left or right is not provided in the columnPinning and pagination state.

KevinVandy commented 4 months ago

@anxhirr I'm fixing the filters example. Also, you should be able to use pnpm test and expect tests to pass after my next commit.

KevinVandy commented 4 months ago

@anxhirr Take a look at my adapter changes. I made it mostly mirror the react-table adapter. I don't know much about Qwik stores vs signals vs whatever, or why NoSerialize is needed here, but it seems to be working better, especially with the initial State.

React on Left. Qwik on Right:

image
anxhirr commented 4 months ago

@KevinVandy I made some other small changes, feel free to look.

I switched store to signal since Qwik store is supposed to be used when storing multiple states/signals inside the same object, which in this case we don't.

Also, noSerialize is necessary since table core is a third part lib and Qwik can not serialize($) its functions.

anxhirr commented 4 months ago

@KevinVandy And oh, for some reason pnpm test fails on table:test:format on my side. Probably something related to prettier

KevinVandy commented 4 months ago

@anxhirr This is looking solid, (I mean qwik), pardon the pun. If we're confident that these are best practices, we just need to do some documentation setup. There's some small amount of work to do in the "docs" folder in this repo that I'll do quick.

I also have an open PR for TanStack.com that I'll get one of the other maintainers to merge for me soon. https://github.com/TanStack/tanstack.com/pull/196

KevinVandy commented 4 months ago

@anxhirr This can be a future PR after this one, but would you be interested in forking the "React Table State" docs for what is needed for Qwik? https://tanstack.com/table/latest/docs/framework/react/guide/table-state

Probably as simple as replacing React.useState with Qwik.useSignal in a few places, I imagine.

KevinVandy commented 4 months ago

@anxhirr What is this error in each of the examples?

main.tsx:202 QWIK ERROR One of the children of is not an accepted value. JSX children must be either: string, boolean, number, , Array, undefined/null, or a Promise/Signal. Instead, it's a function named "header". Error: One of the children of is not an accepted value. JSX children must be either: string, boolean, number, , Array, undefined/null, or a Promise/Signal. Instead, it's a function named "header".

anxhirr commented 4 months ago

@anxhirr What is this error in each of the examples?

main.tsx:202 QWIK ERROR One of the children of is not an accepted value. JSX children must be either: string, boolean, number, , Array, undefined/null, or a Promise/Signal. Instead, it's a function named "header". Error: One of the children of is not an accepted value. JSX children must be either: string, boolean, number, , Array, undefined/null, or a Promise/Signal. Instead, it's a function named "header".

Just checked, and fixed, very strange tho why it did not appear on my side before I did the commit

anxhirr commented 4 months ago

@anxhirr This can be a future PR after this one, but would you be interested in forking the "React Table State" docs for what is needed for Qwik? https://tanstack.com/table/latest/docs/framework/react/guide/table-state

Probably as simple as replacing React.useState with Qwik.useSignal in a few places, I imagine.

@KevinVandy For sure I can help with docs. This weekend I might have plenty of free time

KevinVandy commented 4 months ago

@anxhirr I'm actually not sure if state should be in a useSignal or useStore still. The state is a bunch of nested objects.

I'm guessing the only difference is that useStore would re-render even if you do something like columnPinning.left.push(column.id), while the current useSignal will not. I guess useSignal is safer (from a react dev's perspective).

This can, of course, change later if needed.

anxhirr commented 4 months ago

@anxhirr I'm actually not sure if state should be in a useSignal or useStore still. The state is a bunch of nested objects.

I'm guessing the only difference is that useStore would re-render even if you do something like columnPinning.left.push(column.id), while the current useSignal will not. I guess useSignal is safer (from a react dev's perspective).

This can, of course, change later if needed.

Hi, Yeah I'm not a Qwik expert either but changing it in the future based on needs would not be a breaking change I think.

I see this was merged. Do I need to change/add any docs or you handled it all by yourself?

KevinVandy commented 4 months ago

I see this was merged. Do I need to change/add any docs or you handled it all by yourself?

Any holes in the docs, especially more examples, are welcome to PRs always.

thejanasatan commented 4 months ago

Hello! Trying this out in a hobby project. First of all - thank you for contributing the adapter. Looks like I'm running into a crash in dev mode. Looks like it's related to the use of useSignal / useStore inside this hook. I'm new to Qwik myself so not sure if that's accurate - just going by the error message.

10:31:13 am [vite] Internal server error: Code(20) https://github.com/BuilderIO/qwik/blob/main/packages/qwik/src/core/error/error.ts#L28
  File: /work/src/routes/(app)/orders/current/index.tsx:17:13
  15 |  const Table = component$(() => {
  16 |    
  17 |    const t = useQwikTable({
     |               ^
  18 |      data: [],
  19 |      columns: [],
KevinVandy commented 4 months ago

Create an issue with a sandbox reproducing this error?

thejanasatan commented 4 months ago

https://github.com/TanStack/table/issues/5455 done :+1: Thanks again.