feat(angular-table): Improve proxy signal implementation for v9 - Maybe also v8 #5816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 10e1fce. 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. |
riccardoperra
force-pushed
the
tanstack_v9_angular
branch
2 times, most recently
from
e8e18d9
to
6cebaa7
Compare
riccardoperra
changed the title
riccardoperra
changed the title
riccardoperra
force-pushed
the
tanstack_v9_angular
branch
from
6cebaa7
to
12d0606
Compare
KevinVandy
approved these changes
Nov 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This starts from #5817
Improving proxy implementation and reactive properties
@KevinVandy I've had some time to investigate about the current proxy implementation and some change detection issues we have sometimes in our applications related to how we pass the table instance in sub components and how's FlexRender works.
Currently it's working in most of cases, but depending on how you compose the table, sometimes you lose the ability to use reactive values:
tableobject fromcreateAngularTable, you're fine.directivesand components to extend it's behavior.getContextfn), it happens that those are not updated in the UI (e.g. Angular: Issue with FlexRenderComponent not updating row references in context (injectFlexRenderContext) #5767)(row|cell|header).getContextresult table andcreateAngularTableare different objects. The first one is the plain table (non-reactive), the second one a reactive proxy or a signal.In the current branch I've experimented with the table custom features, and I saw we could use it to make all objects reactive. Since I saw that also the v8 supports custom features, I suppose that could be done on that version too.
https://github.com/riccardoperra/table/blob/tanstack_v9_angular/packages/angular-table/src/reactivity.ts
Basically it's the same as the current proxy implementation, so we wrap the concerned properties into a computed. For testing purposes i've added a
enableExperimentalReactivityproperty while creating the table that enable this new behavior.The only thing that doesn't convince me about this is the heaviness. Depending on the size of the table, creating a reactive prop for every get* may bet slow, since it will be done also for every row/cell/header etc. With debugTable log perf it currently doesn't seem that heavy, but surely we could find a way to detect (also manually) which properties should be considered reactive
Note
This should not be a noticeable breaking change, but if we want to port this on v8 we could make it optional so that we don't risk to break anything. We'll wait for feedback, and then in v9 we could make it the default. Anyway the related issue (#5767) can be fixed without that, but the resolution is less performant performant and I'm not 100% sure it solves the problem (the fix is here)