TanStack / table

🤖 Headless UI for building powerful tables & datagrids for TS/JS - React-Table, Vue-Table, Solid-Table, Svelte-Table
https://tanstack.com/table
MIT License
24.51k stars 3.03k forks source link

feat(angular-table): improve implementation and cleanup code #5518

Closed riccardoperra closed 2 months ago

riccardoperra commented 2 months ago

With this pr i'm fixing the failing build for #5432. I'm also adding the remaining examples and ultimating the proxy implementation which it has been simplified and seems to work for all cases.

There are a lot of changes since I updated the branch with origin/main

nx-cloud[bot] commented 2 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f651c6cf9d5ed00d7c05f1e9260f760acab664b0. 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 - [`nx affected --targets=test:format,test:sherif,test:knip,test:lib,test:types,build --parallel=3`](https://cloud.nx.app/runs/ja94lDZiRT?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

KevinVandy commented 2 months ago

These changes got merged into the main feat-angular-table branch in the tanstack repo by my own git push mistake @riccardoperra , but I'm good to develop from there. If you fork that PR again though, could you do me a favor and give it a different branch name?

merto20 commented 2 months ago

This doesn't seem to work if createAngularTable is called in different context, such as computed. I have the fix ready. Should I commit to this branch or I need to create a different branch? image

riccardoperra commented 2 months ago

@merto20 this is not the expected usage in my opinion. With this approach you'll call createAngularTable on every data change, which will re-create a new instance of table. It's like using toSignal or toObservable inside computed/effect, which is discouraged.

You should call createAngularTable once and pass the signal value inside it. As I understood @tanstack/table-core works using the same table instance, and every options/state update should change table options or it's state

data = signal<Person[]>([])

table = createAngularTable(() => ({
  data: this.data()
}))
merto20 commented 2 months ago

@riccardoperra createAngularTable should have no hard rules how we can use it. If it fits my requirement, then I should be able to do it. Another example is if I call createAngularTable inside ngOnInit or ngAfterViewInit, the same error will happen. You can follow injectQuery or other TanstackQuery methods.

riccardoperra commented 2 months ago

@riccardoperra createAngularTable should have no hard rules how we can use it

This adapter follow the same rules as the other ones, and it works the way angular signals expect to be

another example is if I call createAngularTable inside ngOnInit or ngAfterViewInit, the same error will happen. You can follow injectQuery or other TanstackQuery methods.

Angular Query does the same, since it follows the fetch as you render mental model. You create the query instance through a field initializer/constructor, then pass the reactive data inside the callback

readonly query = injectQuery(() => ({
  queryKey: [this.data()] // this is a signal
})

If you want to use it outside, and in your case if you want to use a computed to store the table, which I think is wrong (you're recreating from scratch the table instance), you can do something like that.

Using computed

  table = computed(() =>
    runInInjectionContext(this.injector, () =>
      createAngularTable(() => ({
        data: this.data(),
        columns: defaultColumns,
        getCoreRowModel: getCoreRowModel(),
        debugTable: true,
      }))
    )
  )

Using ngOnInit/other hooks

export class AppComponent implements OnInit {
  readonly injector = inject(Injector);
  data = signal<Person[]>(defaultData)

  table?: Table<Person>

  ngOnInit() {
    runInInjectionContext(this.injector, () => {
      this.table = createAngularTable(() => ({
        data: this.data(),
        columns: defaultColumns,
        getCoreRowModel: getCoreRowModel(),
        debugTable: true,
      }));
    })
  }
}

Note that this is the same behavior you encounter if you do something like toObservable/toSignal/effect inside a computed. Those are functions that you can use only inside an injection context, then you've to wrap them into runInInjectionContext if you want to use them outside constructor, factory function, field initializer.

Surely before publishing the adapter we should write the documentation and highlight that createAngularTable must be executed within an injection context

merto20 commented 2 months ago

@riccardoperra what I meant was you can optionally add injector parameter on createAngularTable method.