-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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(angular-table) refactor flex-renderer to use signal #5566
Conversation
merto20
commented
May 18, 2024
- convert all inputs to signal
- re-render when content is updated (is this expected?)
- make rendering more flexible by handling function based content first
- convert all inputs to signal - re-render when content is updated (is this expected?) - make rendering more flexible by handling function based content first
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 00d2fd9. 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 targetSent with 💌 from NxCloud. |
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. 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 |
@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 |
@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) |
@riccardoperra |
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 |
@riccardoperra i've tried different machine, I'm not able to run any examples using the main branch. tried |
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. |
@KevinVandy I did build all the packages. Even tried Pls add me to the channel, @merto200. thanks |
@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. |
…le component - fix the broken implementation by allowing the consumer to provide the Component Type to be rendered - allowing the consumer to define the input as signal inside the Component to be rendered
@riccardoperra @KevinVandy The reason why I didn't encounter the error when I tested Please check my latest commits. I've removed the |
I've been using signals in directives. This simplifies the implementation as we don't need to care for changeDetection anymore. |
this should not be an issue since Angular table fully utilizes signals. |
I'm a little confused with this PR. This now looks to be a large breaking change. |
@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 |
@KevinVandy we can revisit this PR again if and when we support signal-based This is the PR I created to support signal-based component. #5576 |
Closing, but as mentioned above, concepts from this PR could be pointed to the v9 alpha branch |