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.51k stars 3.03k forks source link

feat(angular-table) refactor flex-renderer to use signal #5566

Closed merto20 closed 1 month ago

merto20 commented 1 month ago
nx-cloud[bot] commented 1 month ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 00d2fd90e7ba1458eaefc553dd25592ccbd22867. 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 1 target - [`nx affected --targets=test:format,test:sherif,test:knip,test:lib,test:types,build --parallel=3`](https://cloud.nx.app/runs/p0cebKkbLi?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

riccardoperra commented 1 month ago

My flex-render implementation volutely used the ""old"" approach due to non-interchangeable differences between signals execution and ngDoCheck/ngOnChanges.

In the previous implementation, we were not re-rendering all from scratch. vcr.clear() was called on every content change, which should be updated only if you update your column definition variable. Instead, we need to mark the view dirty if the context value reference changes.

You need to use ngDoCheck in order mark the view as dirty manually to all rendered templateRefs/components that may have been changed

You can do some test with the row-selection example which is probably broken with this implementation

KevinVandy commented 1 month ago

@merto20 @riccardoperra This seems like a breaking change. Since its still early in this adapter's life, if we see this change as necessary to performance, I'm open to it, but would want to be clear on the trade offs. And before merging, all docs and examples would need to be updated, and we would probably even need a section in the flex render docs explaining the breaking change.

An alternative approach we could take, would be to export a new signalFlexRender (or similar) method with the different implementation suggested here. No breaking changes, but an alternative that's available.

riccardoperra commented 1 month ago

@KevinVandy Using signals input or decoration based approach doesn't change how the consumer uses the flexRenderDirective, the signature is the same.

What really differs is when the component renders and when it's updated. I didn't notice any benefit at all having signals with flexRender, the view is marked as dirty later and you may have ui synchronization issues, unless you handle it manually (which you do anyway with the other approach)

In my company we have something similiar to the flexRender directive that we use to render with the same api components/templates/strings/functions and we had no performance issue at all using ngDoCheck/ngOnChanges. Even the official angular directive implementation still relies on that approach (ng_template_outlet.ts and ng_component_outlet.ts) which is extensively tested

This is the reason I would prefer to have only the decorator-based approach (which doesn't change nothing for the consumer compared to using signals)

merto20 commented 1 month ago

@riccardoperra row-selection example project is not working on my side even without my changes. Is it working for you?

riccardoperra commented 1 month ago

There are maybe some cache issues running the app in dev. you can check the stackblitz example in the docs to see the right behavior https://tanstack.com/table/latest/docs/framework/angular/examples/row-selection

merto20 commented 1 month ago

@riccardoperra i've tried different machine, I'm not able to run any examples using the main branch. tried --skip-nx-cache option while building the packages but no avail.

KevinVandy commented 1 month ago

You have to build all packages before running examples. We can discuss in the TanStack discord if you are still running into any issues. We can update the contributing guide too if it's not accurate.

merto20 commented 1 month ago

@KevinVandy I did build all the packages. Even tried --skip-nx-cache command thinking it was due to caching issue.

Pls add me to the channel, @merto200. thanks

merto20 commented 1 month ago

@KevinVandy i'm able to run the examples now using a different linux distro. I don't how it happened, but it is a pain when your dev environment got affected like that.

merto20 commented 1 month ago

@riccardoperra @KevinVandy The reason why I didn't encounter the error when I tested grouping example project was because it didn't able to reload using the latest packages build. Now, i have to manually clear the cache before running the example project again to make sure it loads the latest changes.

Please check my latest commits. I've removed the FlexRenderComponent and injectFlexRenderContext. This is a significant change that simplifies the component creation process. By allowing consumers to use signal inputs and component types directly, we're streamlining the development workflow and potentially reducing the learning curve. It's always beneficial to have a more natural and intuitive way of building components, as it can lead to better maintainability and easier collaboration.

merto20 commented 1 month ago

My flex-render implementation volutely used the ""old"" approach due to non-interchangeable differences between signals execution and ngDoCheck/ngOnChanges.

In the previous implementation, we were not re-rendering all from scratch. vcr.clear() was called on every content change, which should be updated only if you update your column definition variable. Instead, we need to mark the view dirty if the context value reference changes.

You need to use ngDoCheck in order mark the view as dirty manually to all rendered templateRefs/components that may have been changed

You can do some test with the row-selection example which is probably broken with this implementation

I've been using signals in directives. This simplifies the implementation as we don't need to care for changeDetection anymore.

merto20 commented 1 month ago

What really differs is when the component renders and when it's updated. I didn't notice any benefit at all having signals with flexRender, the view is marked as dirty later and you may have ui synchronization issues, unless you handle it manually (which you do anyway with the other approach)

this should not be an issue since Angular table fully utilizes signals.

KevinVandy commented 1 month ago

I'm a little confused with this PR. This now looks to be a large breaking change.

merto20 commented 1 month ago

@KevinVandy @riccardoperra Apologies for the oversight, I've just noticed that the Angular Table has already been shipped. To address this, I'm going to break the current PR into two separate ones. I will introduce support for custom component-based rendering with signal inputs, another option to using FlexRendererComponent and injectFlexRenderContext approach.

merto20 commented 1 month ago

@KevinVandy we can revisit this PR again if and when we support signal-based flex-renderer-directive.

This is the PR I created to support signal-based component. https://github.com/TanStack/table/pull/5576

KevinVandy commented 1 month ago

Closing, but as mentioned above, concepts from this PR could be pointed to the v9 alpha branch