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(angular-table) refactor flex-renderer to use signal #5566

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

merto20
Copy link
Contributor

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

nx-cloud bot commented May 18, 2024

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

@riccardoperra
Copy link
Contributor

riccardoperra commented May 18, 2024

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

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

riccardoperra commented May 18, 2024

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

merto20 commented May 18, 2024

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

@riccardoperra
Copy link
Contributor

riccardoperra commented May 18, 2024

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

merto20 commented May 20, 2024

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

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

merto20 commented May 21, 2024

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

merto20 commented May 21, 2024

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

merto20 commented May 21, 2024

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

merto20 commented May 21, 2024

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

merto20 commented May 21, 2024

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

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

@merto20
Copy link
Contributor Author

merto20 commented May 22, 2024

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

merto20 commented May 22, 2024

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

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