feat(angular-table): added injector optional parameter for more flexibility #5525
Conversation
* Added optional injector for more flexibility * Replace runInInjectionContext with injector in effect
|
Should this PR be made into the feat-angular-table branch instead of main? |
|
Can you help review this @riccardoperra? |
packages/angular-table/src/index.ts
Outdated
| let firstRender = true | ||
| effect(() => { | ||
| void [state(), resolvedOptionsSignal()] | ||
| if (firstRender) { | ||
| return (firstRender = false) | ||
| } | ||
| untracked(() => { | ||
| updateOptions() | ||
| notifier.set([]) | ||
| }) | ||
| }) | ||
| // set table options again when options are updated | ||
| effect(() => updateOptions(), { injector }) |
There was a problem hiding this comment.
Consider that this portion of code was present in order to avoid the twice setOptions call during the first table initialization.
There was a problem hiding this comment.
'updateOptions' will be triggered twice everytime options is changed from outside. This is because 'effect' is listening to 2 signals. I was thinking of handling in all scenarios, if we want to fix this, instead of handling only the initialisation part.
There was a problem hiding this comment.
Yeah you're right. I've also seen that currently the svelte adapter has the issue, since it's subscribing to each options and states.
|
I started a thread in the TanStack discord where this can be discussed more: https://discord.com/channels/719702312431386674/1003326939899113492/1236741193619083355 |
|
@KevinVandy @merto20 I approved these changes but I just tried the "signal input" example which seems broken. State is de-syncronized with the latest changes in this branch. I assume this is an issue related to the angular-table-state-bug.mp4I tried to add again again the |
|
@merto20 @riccardoperra So all conversations in this PR should be resolved? We're on the same page here? One other thing, is that I want to make sure that the Angular table state docs are still accurate after these changes, or if we need to add more there to explain alternative approaches to state and signals. Either of you want to help there? |
|
I would wait before merging this, the About the doc, the api changed a little (we now have to pass a function to the adapter), some examples should be rewrited. I may can look on it tomorrow evening |
|
Ok, we'll wait for that regression fix, or if you know the exact fix, @riccardoperra , we can merge this and then a subsequent PR to fix that issue. |
|
@riccardoperra thanks for the video. I'm finishing up the fix for multiple |
*set table options inside computed before returning the table instance *remove redundant signals and effect *remove injector as it no longer required *update Grouping example to show how to pass signal when creating table
|
@riccardoperra I updated the |
| // convert table instance to signal for proxify to listen to any table state and options changes | ||
| const tableSignal = computed( | ||
| () => { | ||
| table.setOptions(updatedOptions()) |
There was a problem hiding this comment.
I’d go back with the previous implementation, updating the options inside a signal is theoretically wrong
There was a problem hiding this comment.
I disagree. Doing side effects not related to value being returned by the computed signal is theoritically wrong. But updating the object before it will be return inside a computed is not. IMHO
There was a problem hiding this comment.
I wanted to get rid of the effect from the start because of its high requirements. But was in doubt because I was not so sure if I'm doing it wrong to call updateOptions method inside a computed. Adding a updatedOptions signal simplifies that and giving more clarity that it is not wrong to update the Table object before it will be returned in the computed signal. Calling updateOptions functions inside the computed signal seems wrong because we cannot control if someone modifies that method and do something more than updating the table options.
There was a problem hiding this comment.
Got it, thanks for the explaination
|
@KevinVandy I htink this is ready to be merged 😄 |
* feat(angular-table): adds Angular adapter for tanstack/table (#5326) * Angular adapter for tanstack table initial * build with: ng packagr * Add angular examples basic grouping * Add angular examples basic grouping * Update angular examples basic grouping * Add angular example selection * regen lock file * package upgrades, angular table docs * prettier * move around some deps * removed unused dependency from angular package * fix deps --------- Co-authored-by: Kevin Van Cott <kevinvandy656@gmail.com> * docs config cleanup * feat: signal angular table adapter implementation * update demo * feat: table proxy detect memoized fns * fix proxy property returning value * feat: improve naming * save new reference of table signal on every update * computed trap proxy for fns with 1 argument * update pnpm-lock.yaml * refactor proxy implementation * fix dependencies * cleanup imports * refactor basic example * fix build * run prettier * add grouping example, fix ci * add grouping example, fix ci * add row selection example * add column visibility example * update examples add signal input required example * improve angular table impl, fix flex-render change detection issues * fix build * feat(angular-table): improve adapter implementation (#5524) * feat(angular-table): support render dynamic components and templateRefs in table * update row selection example using dynamic rendered components * support change detection on push with flex render * add column ordering example * fix flexRender change detection issues * rename properties * fix prettier and adjust example budget options * update basic example * add again support for table signal * add column-pinning example * add column pinning example * add filters example * cleanup code * example cleanup * update lock file * feat(angular-table): added injector optional parameter for more flexibility (#5525) * feat(angular-table): improve adapter implementation (#5524) * Added optional injector for more flexibility * Replace runInInjectionContext with injector in effect * feat(angular-table): Added proxifyTable back * feat(angular-table): adding back notifier signal for table changed * feat(angular-table): Improve logic in setting table options *set table options inside computed before returning the table instance *remove redundant signals and effect *remove injector as it no longer required *update Grouping example to show how to pass signal when creating table * update angular adapter and state docs * prettier * update docs config with angular examples * update angular table state docs (#5545) * Angular examples and dependencies improvements (#5546) * tslib should be a dependency, see: https://angular.io/guide/angular-package-format#tslib * ensure angular examples are run as web container on code sandbox * update lock file --------- Co-authored-by: Kevin Vandy <kevinvandy656@gmail.com> * docs(angular-table): Add documentation for FlexRenderDirective (#5543) * add flexRender documentation * fix docs * fix rendering component section heading * remove double build package.json from angular build * update link name * docs pass * update esm exports in package.json * update flexrender types --------- Co-authored-by: jrgokavalsa <86955546+jrgokavalsa@users.noreply.github.com> Co-authored-by: riccardoperra <riccardo.perra@icloud.com> Co-authored-by: Riccardo Perra <perrariccardo0@gmail.com> Co-authored-by: mamerto-g <merto.20@gmail.com> Co-authored-by: Arnoud <6420061+arnoud-dv@users.noreply.github.com>
Following are the changes:
Added optional
injectorparameter for more flexibility. Consumer cancreateAngularTableanywhere.Replace runInInjectionContext with an injector in the
effect. This has the same behaviour since onlyeffectrequires to be in the injector context.Remove unncessary untracked and signals.
Remove proxifyTable as I think this is unncessary, since table is not required to be a signal.
laziInitalready proxified the table object. Pls correct me if I'm wrong.Refactored grouping example to include pagination state.