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): improve implementation and cleanup code #5518

Merged

Conversation

riccardoperra
Copy link
Contributor

@riccardoperra riccardoperra commented Apr 29, 2024

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

  • Add support for required signal inputs
  • Basic example
  • Grouping example
  • Row selection example
  • Column visibility example
  • Required signal input example
  • Column ordering example
  • Column pinning example
  • Column pinning sticky example
  • Filters

Copy link

nx-cloud bot commented Apr 29, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f651c6c. 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.

@KevinVandy KevinVandy merged commit 38879fd into TanStack:feat-angular-table May 3, 2024
2 checks passed
@KevinVandy
Copy link
Member

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

merto20 commented May 4, 2024

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

riccardoperra commented May 4, 2024

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

merto20 commented May 4, 2024

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

riccardoperra commented May 4, 2024

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

merto20 commented May 5, 2024

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

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

9 participants