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
25.01k stars 3.07k forks source link

docs(angular-table) Angular table state docs #5545

Closed riccardoperra closed 4 months ago

riccardoperra commented 4 months ago

I've updated the table state docs according to the angular implementation.

I had to refactor a bit the example of the Individual Controlled State combining rxjs and signals instead of using angular-query, but I'm not sure about that. It's certainly a good way to make Angular Query known, but at the moment it's a library with the experimental flag. I didn't have any issue using it but I don't know if everything is clear to a user who sees this documentation

I would like to get also some feedback from @merto20 about my fully controlled state approach, which in my opinion could differ from other implementations due to some constraints.

This is what the other frameworks does:

const table = createSolidTable({
  columns,
  get data() {
    return data()
  },
  //... Note: `state` values are NOT passed in yet
})

const [state, setState] = createSignal({
  ...table.initialState
})

// **This is our constructor**
table.setOptions(prev => ({
  ...prev,
  get state() {
    return state()
  },
  onStateChange: setState
}))

This is what I did

class TableComponent {
  // create an empty table state, we'll override it later
  readonly state = signal({} as TableState);

  // create a table instance with default state values
  readonly table = createAngularTable(() => ({
    columns: this.columns,
    data: this.data(),
    // our fully controlled state overrides the internal state
    state: this.state(),
    onStateChange: updater => {
      // any state changes will be pushed up to our own state management
      this.state.set(
        updater instanceof Function ? updater(this.state()) : updater
      )
    }
  }))

  constructor() {
    // set the initial table state
    this.state.set({
      // populate the initial state with all of the default state values
      // from the table instance
      ...this.table.initialState,
      pagination: {
        pageIndex: 0,
        pageSize: 15, // optionally customize the initial pagination state.
      },
    })
  }
}

In angular, we may have to pass both state and onStateChange properties into the createAngularTable initialization. Using setOptions that introduces a new signal into the dependency graph breaks our rules to mark the view dirty. So the signal will be updated outside, but it's not reflected into the view and the proxified properties

nx-cloud[bot] commented 4 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4182c74d1090d2b62a0cbc8dfd41101ceac415b9. 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/EgHv5rxwrQ?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

riccardoperra commented 4 months ago

In angular, we may have to pass both state and onStateChange properties into the createAngularTable initialization. Using setOptions that introduces a new signal into the dependency graph breaks our rules to mark the view dirty. So the signal will be updated outside, but it's not reflected into the view and the proxified properties

@KevinVandy @merto20 About this, I was thinking of a solution that could be implemented but may require some effort. We should override the setOptions (and maybe add an implementation of mergeOptions) which looks for all reactive properties (getters?), and handle them notifying our signals in orders to mark the view as dirty.

Example:


notifier = signal({});

mergeOptions(prevOptions, nextOptions) { 
  const mergedOptions = {...prevOptions};
  for (const property in nextOptions) { 
     const getter = Object.getOwnPropertyDescriptor(nextOptions, property);
     if (!getter) { 
        Object.defineProperty(mergedOptions, property, { value : nextOptions[property] };
     } else { 
        Object.defineProperty(mergedOptions, property, {
            get() {
                notifier.set({});
                return getter();
            }
        })
     }
  }

  return mergedOptions;
}

This example is simplified, doing a ready implementation like the vue/solid one (which uses solid-js mergeOptions) may require some time and could cause more issues than it solves.

Considering that we currently can handle the fully-controlled state via signals during the initialization, in my opinion we don't necessarily have to support this now and we may can look into it after the release, get feedback from consumers and then evaluate whether it makes sense to do .

Following the other framework approach, we should support something like this:


class Component {
  readonly table = createAngularTable(() => {});

  readonly state = signal({...table.initialState})
  constructor() { 
    const {state} = this;
    effect(() => { 
       table.setOptions(prev => ({
          ...prev,
          get state() {
            return state();
         }
       })
    })
  }
}
KevinVandy commented 4 months ago

We should eventually just make the "fully controlled" example like the react one to go along with these docs. Our docs can evolve.

merto20 commented 4 months ago

@KevinVandy @riccardoperra Is it sensible to convert all documentation and examples to utilize signals? I am considering this if it proves to be a logical step.

riccardoperra commented 4 months ago

Isn't the doc already covering examples using signals?

merto20 commented 4 months ago

I've updated the table state docs according to the angular implementation.

I had to refactor a bit the example of the Individual Controlled State combining rxjs and signals instead of using angular-query, but I'm not sure about that. It's certainly a good way to make Angular Query known, but at the moment it's a library with the experimental flag. I didn't have any issue using it but I don't know if everything is clear to a user who sees this documentation

I would like to get also some feedback from @merto20 about my fully controlled state approach, which in my opinion could differ from other implementations due to some constraints.

This is what the other frameworks does:

const table = createSolidTable({
  columns,
  get data() {
    return data()
  },
  //... Note: `state` values are NOT passed in yet
})

const [state, setState] = createSignal({
  ...table.initialState
})

// **This is our constructor**
table.setOptions(prev => ({
  ...prev,
  get state() {
    return state()
  },
  onStateChange: setState
}))

This is what I did

class TableComponent {
  // create an empty table state, we'll override it later
  readonly state = signal({} as TableState);

  // create a table instance with default state values
  readonly table = createAngularTable(() => ({
    columns: this.columns,
    data: this.data(),
    // our fully controlled state overrides the internal state
    state: this.state(),
    onStateChange: updater => {
      // any state changes will be pushed up to our own state management
      this.state.set(
        updater instanceof Function ? updater(this.state()) : updater
      )
    }
  }))

  constructor() {
    // set the initial table state
    this.state.set({
      // populate the initial state with all of the default state values
      // from the table instance
      ...this.table.initialState,
      pagination: {
        pageIndex: 0,
        pageSize: 15, // optionally customize the initial pagination state.
      },
    })
  }
}

In angular, we may have to pass both state and onStateChange properties into the createAngularTable initialization. Using setOptions that introduces a new signal into the dependency graph breaks our rules to mark the view dirty. So the signal will be updated outside, but it's not reflected into the view and the proxified properties

I would re-write the example to this:

class TableComponent {
  readonly pagination = signal<PaginationState>({ pageIndex: 0, pageSize: 15 })

  tableOptions = computed(() => ({
    data: this.data(),
    columns: columns,
    state: {
      pagination: this.pagination(),
    },
  }))

  table = createAngularTable(this.tableOptions)

  constructor() {}

  onNextPage() {
    const pagination = this.pagination()
    if (pagination.pageIndex < pagination.pageSize) {
      pagination.pageIndex++
      this.pagination.set({ ...pagination })
    }
  }

  onPrevPage() {
    const pagination = this.pagination()
    if (pagination.pageIndex > 0) {
      pagination.pageIndex--
      this.pagination.set({ ...pagination })
    }
  }
}

Passing the onStateChange handler when invoking createAngularTable seems unnecessary. Ideally, examples should avoid suggesting the use of setOptions or table.initialState, as these are managed internally.

riccardoperra commented 4 months ago

You use onStateChange/state in order to have a single object with the entire state of the table, which allows you to have a full state controlled approach. We already have an example to individually handle the state (https://tanstack.com/table/latest/docs/framework/angular/guide/table-state#individual-controlled-state) which is ported from the other adapters.

Also, you may want to use next/back methods from tanstack table to update your signals in order to have an already working implementation to update the state instead of re-writing another one from scratch, so you'll end up to use the onStateChange/onPaginationChange handlers to synchronize them.

merto20 commented 4 months ago

I specifically mention onStateChange, not the individual state, since we are already handling the entire state internally. For me, I don't see any necessity handling the onStateChange when I create table instance.

We are always re-writing the table options object, that's the only way we notify the table instance to update the options internally. The computed that listens to options will not be notified if its object address stays the same.

I use pagination as my example as I can control the values of my pagination and inform the table about the changes of the pagination. The flow is only 1 way, since the change is coming from my component not from the table. If I set the pagination change using table.setPageIndex, the flow is routed from the table, then to my component (through onPaginationChange handle) and going back to the table.