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): angular adapter implementation with signals #5442

Merged

Conversation

riccardoperra
Copy link
Contributor

@riccardoperra riccardoperra commented Mar 26, 2024

This is still a wip implementation of the angular adapter with a mixed approach:

The adapter will always return a signal table. This is useful if passed as a component input. That signal change on every state/option change. If a signal has been used within the options, the table object will be updated automatically.

@Component({
  selector: 'app-root',
  standalone: true,
  imports: [FilterComponent, FlexRenderDirective],
  template : `
    <table>
      <thead>
        @for (headerGroup of table().getHeaderGroups(); track headerGroup.id) {
          <tr>
            @for (header of headerGroup.headers; track header.id) {
              ...
            }
          </tr>
        }
      </thead>
      <tbody>
        @for (row of table().getRowModel().rows; track row.id) {
          ...
        }
      </tbody>
    </table>
  `,
  styleUrl: './app.component.scss',
})
export class AppComponent {
  readonly data = signal([]);

  // This will re-run on every "data" change
  readonly table = createAngularTable(() => ({
    data: this.data,
    columns: this.columns,
  })
}

I've also added in this pr the proxy implementation, where the signal it's extended with a proxified object that allows state slicing. This may not be useful for the adapter, I'm not sure about this and and it would be nice to have some feedback, but maybe "it's too much work" and it's a pain to mantain.

Also, we need to check if there are some caveats with that approach. Internally I'm using computed which memoize the value by default, so in case the view will marked as dirty only if the value instance change (Object.is). This could be a premature optimization considering that almost every property would be transformed.

The first one (signal-only) is safer, and even with it you can do "state slicing", you just need to manually use computed.

class HelloComponent { 
  readonly data = signal([]);
  readonly pagination = signal<PaginationState>({pageIndex: 0, pageSize: 20});

  // This will re-run on every "data" and "pagination" change
  readonly table = createAngularTable(() => ({
    data: this.data,
    columns: this.columns,
    state: {
      pagination: this.pagination()
    },
    onPaginationChange: (updater) => {
      this.pagination.set(...)
    }
    // [..]
  }));

  constructor() {
    effect(() => {
      // run on every state/option change
      this.table().getPageOptions();
    });

    // more granular state, run on every "getPageOptions" change (e.g. only when pagination state change)
    effect(() => {
      this.table.getPageOptions()
    });

    // granular state is still possible using computed, but it must be done manually
    const pageOptions = computed(() => this.table().getPageOptions());
    effect(() => {
      pageOptions()
    });

    // these two are lines does the same thing, they access to the same table instance property. 
    this.table().setPageSize(...)
    this.table.setPageSize() // this one is evaluated lazily with proxy
  }
}

I've updated demo code but I may opt to make them simpler as was done for the others. Also, the unit/integration test part is missing. Note also that this implementation doesn't cover required inputs signals (need to wrap with lazy-signal-initializer / lazy-init as described here)

@KevinVandy
Copy link
Member

@riccardoperra Wondering how much of your proxy can be replicated with TanStack Angular Store: https://tanstack.com/store/latest/docs/framework/angular/quick-start. I originally thought we'd have to wait until TanStack Table v9 to introduce state subscriptions, but this looks interesting.

@riccardoperra
Copy link
Contributor Author

riccardoperra commented Mar 27, 2024

Not sure about that. Hope I understood, but In this case I think the table signal could be replaced into a store

- const table = signal(createTable(resolvedOptionsSignal()), {
-  equal: () => false,
- })
+ const table = new Store(createTable(resolvedOptionsSignal()))

then in the effect we should update the store instead of the signal

- table.update(v => v)
+ table.setState(...)

Then for each property that should be treated as a "slice", we should replace the computed with the injectStore

https://github.com/TanStack/table/pull/5442/files#diff-a3500075d578d363191c8bc27e94c7f42a2426e4b6e5c582551cfafe9d2576b4R46-R50

value = injectStore(table, (state) => state[property](), { injector })

However, I think this implicate that for each property a new writeable signal and an effect will be created, wouldn't it be less performant / consuming more memory than a computed fn?

@KevinVandy
Copy link
Member

I'd like to get one of these approaches in the main PR ASAP so that we can march forward on the rest of it with signals. Thoughts @crutchcorn when you have time?

@riccardoperra
Copy link
Contributor Author

riccardoperra commented Mar 27, 2024

Not sure having an approach where the table is componentized is recommended, (does the other frameworks do this aswell?.

@Component({
  selector: 'my-user-table',
  template: `
    @if (table(); as table) {
       ...
    }
  `,
  standalone: true,
})
class MyUserTableComponent {
  table = input.required<TableResult>();
}

@Component({
  template: `
    <my-user-table [table]="table()/>
 `
})
class AppComponent {
  table = createAngularTable(() => ({
    data: this.data
  });

  constructor() {
    this.http.get('...').subscribe(() => this.data.set(data))
  }
}

Is it better to create the table instance inside the table component and, for example, just pass the data as input?

Anyway, I had to update the signal code approach due to some failing test: even with equals: () => false, the view is not updated if the Table is passed as an input (basically, you'll see empty data even if it's present)

@KevinVandy
Copy link
Member

Not sure having an approach where the table is componentized is recommended, (does the other frameworks do this as well?.

@riccardoperra I'm not really sure what that means. But if it means passing the table instance around to components as a param/prop, that's common. https://github.com/KevinVandy/material-react-table/blob/v2/packages/material-react-table/src/components/table/MRT_Table.tsx

@KevinVandy
Copy link
Member

Once a direction has been chosen, let's mark this as ready to review and merge to the main PR

@KevinVandy KevinVandy merged commit d0bd1dc into TanStack:feat-angular-table Mar 28, 2024
@KevinVandy
Copy link
Member

this is weird. I don't remember merging this, but it's merged into my PR. @riccardoperra can we move forward with one of the signal adapters you proposed this week and keep working on it?

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.

2 participants