Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: qwik-table adapter #5420

Merged
merged 19 commits into from Mar 22, 2024
Merged

Feat: qwik-table adapter #5420

merged 19 commits into from Mar 22, 2024

Conversation

anxhirr
Copy link
Contributor

@anxhirr anxhirr commented Mar 18, 2024

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: #5281

@KevinVandy
Copy link
Member

Looks good, will probably test this out tomorrow!

@anxhirr
Copy link
Contributor Author

anxhirr commented Mar 19, 2024

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.

Copy link

nx-cloud bot commented Mar 20, 2024

☁️ Nx Cloud Report

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

Sent with 💌 from NxCloud.

@KevinVandy
Copy link
Member

@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
Copy link
Contributor Author

anxhirr commented Mar 20, 2024

@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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

anxhirr commented Mar 21, 2024

@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
Copy link
Contributor Author

anxhirr commented Mar 21, 2024

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

@KevinVandy
Copy link
Member

@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. TanStack/tanstack.com#196

@KevinVandy
Copy link
Member

KevinVandy commented Mar 21, 2024

@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
Copy link
Member

@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
Copy link
Contributor Author

anxhirr commented Mar 21, 2024

@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
Copy link
Contributor Author

anxhirr commented Mar 21, 2024

@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
Copy link
Member

@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.

@KevinVandy KevinVandy merged commit f99f500 into TanStack:main Mar 22, 2024
2 checks passed
@anxhirr
Copy link
Contributor Author

anxhirr commented Mar 22, 2024

@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
Copy link
Member

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
Copy link

thejanasatan commented Mar 29, 2024

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
Copy link
Member

Create an issue with a sandbox reproducing this error?

@thejanasatan
Copy link

#5455
done 👍 Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants