feat(angular-table): Added support for custom component renderer with signal

[merto20]

This support provides option to simplify the component creation process. By allowing consumers to use signal inputs and component types directly.

[nx-cloud]

☁️ Nx Cloud Report

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

---

Sent with 💌 from NxCloud.

[merto20]

@KevinVandy @riccardoperra this change allows the following:

Defining columns using the Component types directly.

    readonly columns: ColumnDef<Person>[] = [
    {
      id: 'select',
      header: () => TableHeadSelectionComponent<Person>,
      cell: () => TableRowSelectionComponent<Person>,
    },

Using input signals in the Component.

@Component({
  template: `
    <input
      type="checkbox"
      [checked]="table().getIsAllRowsSelected()"
      [indeterminate]="table().getIsSomeRowsSelected()"
      (change)="table().toggleAllRowsSelected()"
    />
  `,
  host: {
    class: 'px-1 block',
  },
  standalone: true,
  changeDetection: ChangeDetectionStrategy.OnPush,
})
export class TableHeadSelectionComponent<T> {
  // Your component should also reflect the fields you use as props in flexRenderer directive.
  // Define the fields as input you want to use in your component
  // ie. In this case, you are passing HeaderContext object as props in flexRenderer directive.
  // You can define and use the table field, which is defined in HeaderContext.
  // Please take note that only signal based input is supported.

  //column = input.required<Column<T, unknown>>()
  //header = input.required<Header<T, unknown>>()
  table = input.required<Table<T>>()
}

[riccardoperra]

Providers allows to inject the context and use the variables immediately (you don't need to wait for onInit+ or using effects);

I have some doubts about relying only on inputs:

• Inputs are memoized by their reference value, which means that if the table/row/column instance doesn't change, view for the component is not marked as dirty by default and you'll encounter synchronization issues. Currently the ui seems fine because in the current *flexRender implementation, I manually run the changeDetectionRef#markForCheck on ngDoCheck (which I think we can anyway schedule when the props change)

Currently effect in the TableRowSelectionComponent will trigger only once, since the table instance doesn't change after updating the row selection state. This isn't what the user expect I think.

• Components should have always the same signature and also use the input with the right name. This is not really a problem, could we foresee interfaces that the component implements?

• Without a wrapper you cannot pass a custom injector per component, or you cannot provide your own inputs if you don't pass them as context. These inputs are defined by the user, and may not relate to the table.

[merto20]

Providers allows to inject the context and use the variables immediately (you don't need to wait for onInit+ or using effects);

I think you need to revisit this implementation. If proxy can be used immediately, then you don't need to manually use setInput for each property.

  const componentInjector = Injector.create({
      parent: injector ?? this.injector,
      providers: [{ provide: FlexRenderComponentProps, useValue: proxy }],
    })

    const componentRef = this.viewContainerRef.createComponent(component, {
      injector: componentInjector,
    })
    for (const prop in inputs) {
      if (componentRef.instance?.hasOwnProperty(prop)) {
        componentRef.setInput(prop, inputs[prop])
      }
    }

setInput automatically marks for check the component. Signal inputs will behave as they should.

I have some doubts about relying only on inputs:

• Inputs are memoized by their reference value, which means that if the table/row/column instance doesn't change, view for the component is not marked as dirty by default and you'll encounter synchronization issues. Currently the ui seems fine because in the current *flexRender implementation, I manually run the changeDetectionRef#markForCheck on ngDoCheck (which I think we can anyway schedule when the props change)

This should not be an issue using Signal. As you may know, the framework handles changeDetection automatically when signal changes. Using signal in flexRendererDirective will also have the same benefit. Using signal encourage immutability of data, and since Angular Table already handles converting all table data to signal, then this should not be an issue.

Currently effect in the TableRowSelectionComponent will trigger only once, since the table instance doesn't change after updating the row selection state. This isn't what the user expect I think.

The table won't change but column and header could be. This is the question I have in my first PR, if we have such cases where column defination would change after it is displayed. This could be possible, hence, the current flexRendererDirective cannot handle such cases without handling it manually using ngOnChange or simply using signal inputs.

• Components should have always the same signature and also use the input with the right name. This is not really a problem, could we foresee interfaces that the component implements?
• Without a wrapper you cannot pass a custom injector per component, or you cannot provide your own inputs if you don't pass them as context. These inputs are defined by the user, and may not relate to the table.

Without using wrapper simplifies how components are implemented. Consumer doesn't need to know that they should inject properties with injectFlexRenderContext and use FlexRendererComponent to make their own components work with Angular Table.

Below is how it is used in react. Consumer can simply used all data being passed as props. I think there is nothing wrong doing like this. It is simple and just works.

      {
        id: 'select',
        header: ({ table }) => (
          <IndeterminateCheckbox
            {...{
              checked: table.getIsAllRowsSelected(),
              indeterminate: table.getIsSomeRowsSelected(),
              onChange: table.getToggleAllRowsSelectedHandler(),
            }}
          />
        ),
        cell: ({ row }) => (
          <div className="px-1">
            <IndeterminateCheckbox
              {...{
                checked: row.getIsSelected(),
                disabled: !row.getCanSelect(),
                indeterminate: row.getIsSomeSelected(),
                onChange: row.getToggleSelectedHandler(),
              }}
            />
          </div>
        ),
      },

[riccardoperra]

I think you need to revisit this implementation. If proxy can be used immediately, then you don't need to manually use setInput for each property. I thought as inputs as a way to pass data to components that it's not necessarily related to the table context

Providers are immediately available, the proxy is needed in order to always get the latest value when calling this.context.[myProperty]. Otherwise, you'll always get the first value the context has.

setInput automatically marks for check the component. Signal inputs will behave as they should.

Looking to the source code, if the reference value doesn't change, the component is not marked as dirty https://github.com/angular/angular/blob/825023801baf8dbd0bd87b7ec46a65e869e08adb/packages/core/src/render3/component_ref.ts#L464

The table won't change but column and header could be. This is the question I have in my first PR, if we have such cases where column defination would change after it is displayed. This could be possible, hence, the current flexRendererDirective cannot handle such cases without handling it manually using ngOnChange or simply using signal inputs.

If table doesn't change and we don't handle change detection in that case, calling table.getIsAllRowSelected() etc... will return a non-synchronized value.

Without using wrapper simplifies how components are implemented. Consumer doesn't need to know that they should inject properties with injectFlexRenderContext and use FlexRendererComponent to make their own components work with Angular Table. Below is how it is used in react. Consumer can simply used all data being passed as props. I think there is nothing wrong doing like this. It is simple and just works.

All other frameworks allows to define the component template inside the ts code, then you can set the input as you want. Doing that in angular will force consumers to create a component with exactly that input names.

[merto20]

Looking to the source code, if the reference value doesn't change, the component is not marked as dirty https://github.com/angular/angular/blob/825023801baf8dbd0bd87b7ec46a65e869e08adb/packages/core/src/render3/component_ref.ts#L464

what i meant was the component is marked dirty right after setInput is called. But if your input is a signal, the signal will track the changes of that value and automatically schedule change detection.

If table doesn't change and we don't handle change detection in that case, calling table.getIsAllRowSelected() etc... will return a non-synchronized value.

what do you mean by this? It seems like you're implying that the framework is maintaining multiple versions of the value? I think this is incorrect. If the object reference did not change, but the value of the object did (this means the object is mutated), change detection will not happen (This is the same for signal and ngOnChanges). The value of the object being mutated will still be updated. So the case you mention that table.getIsAllRowSelected() will return a non-synchronized value is incorrect.

The table doesn't change but its states will. When table state changes, Angular table handles that changes, which updates the signal values, which automatically schedule change detection.

All other frameworks allows to define the component template inside the ts code, then you can set the input as you want. Doing that in angular will force consumers to create a component with exactly that input names.

Isn't this better? The consumer knows what type of props it needs for the component. We allow granularity of which props the component needs.

Example below:

@Component({
  template: `
    <input
      type="checkbox"
      [checked]="context.row.getIsSelected()"
      (change)="context.row.getToggleSelectedHandler()($event)"
    />
  `,
  host: {
    class: 'px-1 block',
  },
  standalone: true,
  changeDetection: ChangeDetectionStrategy.OnPush,
})
export class TableRowSelectionComponent<T> {
  context = injectFlexRenderContext<CellContext<T, unknown>>()
}

TableRowSelectionComponent only needs the row object of the props. But we are forcing the component to take the whole CellContext object.

[riccardoperra]

what do you mean by this? It seems like you're implying that the framework is maintaining multiple versions of the value? I think this is incorrect. If the object reference did not change, but the value of the object did (this means the object is mutated), change detection will not happen (This is the same for signal and ngOnChanges). The value of the object being mutated will still be updated. So the case you mention that table.getIsAllRowSelected() will return a non-synchronized value is incorrect.

I wrote quickly and explained myself poorly.

@Component({
  template: `
    <input
      type="checkbox"
      [checked]="table().getIsAllRowsSelected()"
      [indeterminate]="table().getIsSomeRowsSelected()"
      (change)="table().toggleAllRowsSelected()"
    />
  `,
  host: {
    class: 'px-1 block',
  },
  standalone: true,
  changeDetection: ChangeDetectionStrategy.OnPush,
})
export class TableHeadSelectionComponent<T> {
  table = input();

Some premises:

• Tanstack table does not recreate a new instance of the table every time.
• row.getContext() will return everytime a new plain object that contains table/row/column which are not "patched" with signal-proxy

Suppose that we have a table signal that we try to set with componentRef#setInput. If we look to the setInput implementation, if the reference value of our property doesn't change, which seems our case, there is an early return that prevent to update the input and does not mark the view as dirty.

If the view is not marked as dirty, our table().getIsAllRowsSelected() will not be revaluated. So the UI will not always be synchronized with the current state. It will work maybe the first time, but after you update the state from a tbody column, the header component will not marked as dirty and table.getIsAllRowSelected() will not be evaluated again.

In the current implementation, this is now working because we have the ngDoCheck hook which manually mark the view as dirty for the component.

This is what happens if you remove that logic into the flex render and relies only on setInput

tanstack-table-without-ngdocheck.mp4

table/packages/angular-table/src/flex-render.ts

Lines 59 to 65 in a4bd09a

	ngDoCheck() {
	if (this.ref instanceof ComponentRef) {
	this.ref.injector.get(ChangeDetectorRef).markForCheck()
	} else if (this.ref instanceof EmbeddedViewRef) {
	this.ref.markForCheck()
	}
	}

Anyway I think this can be moved into an ngOnChanges, since I think we want to do that only if the props property changes

ngOnChanges(changes) { 
   // mark the current component view as dirty, if present
   componentRef?.injector.get(ChangeDetectorRef).markForCheck();
   
   if (!changes.content) { 
      // if content doesn't change we don't need to render again from scratch
      return
   }

   vcr.clear();
   // rendering logic
   render(this.content, this.props);
}

---

Isn't this better? The consumer knows what type of props it needs for the component. We allow granularity of which props the component needs.

I am ok with you about that without injectContext and directly passing a component will be easier for consumers, but in my opinion a very important use case is that the user should be able to also set the component as he wants, and must be able to pass other inputs as well like other frameworks adapter does.

With a "wrapper" you could do something like that

new FlexRenderComponent(MyComponent, { onClick: ..., isDisabled: this.someActionIsLoading...} )

What we could do to simplify this is maybe do something like hostDirectives, where you pass an object, but I think we miss the type hints, which you can keep with the current FlexRenderComponent

() => {
  return { 
     component: MyComponent,
     inputs: {} 
  }
}

[riccardoperra]

TableRowSelectionComponent only needs the row object of the props. But we are forcing the component to take the whole CellContext object.

You're right, it only needs the row object in our example. But you can't know if the consumer need to use them for it's use cases.

With the inject approach you'll get the same context as the column definition.

Consider that these properties are always present when you define the cell/footer/header def

• https://tanstack.com/table/latest/docs/api/core/column-def#cell
• https://tanstack.com/table/latest/docs/api/core/column-def#footer
• https://tanstack.com/table/latest/docs/api/core/column-def#header

[merto20]

Some premises:

• Tanstack table does not recreate a new instance of the table every time.

Yes, but custom component renderer implementation is using signal input as Inputs because it is available after the component is created using viewContainerRef.createComponent. It is simplier to implement rather than using custom injectFlexRenderContext to make the inputs visible and aligned to our goal to support only signals. Even if the component input is not expected to changed, they can be assigned to an input signal.

• row.getContext() will return everytime a new plain object that contains table/row/column which are not "patched" with signal-proxy

Suppose that we have a table signal that we try to set with componentRef#setInput. If we look to the setInput implementation, if the reference value of our property doesn't change, which seems our case, there is an early return that prevent to update the input and does not mark the view as dirty.
setInput will always make component dirty the first time you set the value. The next value change detection will be handled by signal, for custom component renderer.
In the current implementation, this is now working because we have the ngDoCheck hook which manually mark the view as dirty for the component.

I was having this assumption that we converted all members/object in the table to computed. But when I checked, only the first layers are converted. But the objects returned by methods are not. This is why we need handle change detection in onDoCheck. In below example, if row was proxified as signal we don't need to handle change detection manually.

@Component({
  template: `
    <input
      type="checkbox"
      [checked]="row().getIsSelected()"
      (change)="row().getToggleSelectedHandler()($event)"
    />
  `,
  host: {
    class: 'px-1 block',
  },
  standalone: true,
  changeDetection: ChangeDetectionStrategy.OnPush,
})
export class TableRowSelectionComponent<T> {
  row = input.required<Row<T>>()
}

I am ok with you about that without injectContext and directly passing a component will be easier for consumers, but in my opinion a very important use case is that the user should be able to also set the component as he wants, and must be able to pass other inputs as well like other frameworks adapter does.

I'm ok with this having a wrapper that injects customs inputs. But are we doing it in other frameworks, like react? I didn't see any examples. And also, are there any such cases where we need custom inputs?

Please take note that both directives and components are handled by compiler differently if signals: true.
https://github.com/angular/angular/blob/825023801baf8dbd0bd87b7ec46a65e869e08adb/packages/core/src/render3/jit/directive.ts#L453
https://github.com/angular/angular/blob/825023801baf8dbd0bd87b7ec46a65e869e08adb/packages/compiler/src/core.ts#L39

[merto20]

You're right, it only needs the row object in our example. But you can't know if the consumer need to use them for it's use cases.

With the inject approach you'll get the same context as the column definition.

Consider that these properties are always present when you define the cell/footer/header def

This is why i explained in my example below that consumer can define all the objects the components need from the props. I commented the other header and column inputs because they are not used in the component.

@Component({
  template: `
    <input
      type="checkbox"
      [checked]="table().getIsAllRowsSelected()"
      [indeterminate]="table().getIsSomeRowsSelected()"
      (change)="table().toggleAllRowsSelected()"
    />
  `,
  host: {
    class: 'px-1 block',
  },
  standalone: true,
  changeDetection: ChangeDetectionStrategy.OnPush,
})
export class TableHeadSelectionComponent<T> {
  // Your component should also reflect the fields you use as props in flexRenderer directive.
  // Define the fields as input you want to use in your component
  // ie. In this case, you are passing HeaderContext object as props in flexRenderer directive.
  // You can define and use the table field, which is defined in HeaderContext.
  // Please take note that only signal based input is supported.

  //column = input.required<Column<T, unknown>>()
  //header = input.required<Header<T, unknown>>()
  table = input.required<Table<T>>()
}

[riccardoperra]

I'm ok with this having a wrapper that injects customs inputs. But are we doing it in other frameworks, like react? I didn't see any examples. And also, are there any such cases where we need custom inputs?

This is an example of material-react-table which has defined a custom input to the selection component. They've could also defined table and row with different names. It depends of how the user will creatte the component

https://github.com/KevinVandy/material-react-table/blob/9422d2bbe95316133fada0bd2571a56d6583d8fd/packages/material-react-table/src/hooks/display-columns/getMRT_RowSelectColumnDef.tsx#L4

Unfortunately what's missing in angular is defining an inline template per cell like the other framework does

[merto20]

This is an example of material-react-table which has defined a custom input to the selection component. They've could also defined table and row with different names. It depends of how the user will creatte the component

https://github.com/KevinVandy/material-react-table/blob/9422d2bbe95316133fada0bd2571a56d6583d8fd/packages/material-react-table/src/hooks/display-columns/getMRT_RowSelectColumnDef.tsx#L4

It seems like the only way to pass values to the component is still through props in flexRenderer for react. For Angular it is different, because you are creating new instance of FlexRenderComponent passing the type and inputs. So I still think pasing custom input to components still not necessary. If you want to pass any values, you can simply pass them through props in flexRenderer.

[KevinVandy]

Hey fyi. Still following this PR. PRs into the main branch need to be non breaking. However, we have a newly created alpha branch for v9 that can accept well documented breaking changes.

If we deem any of these proposals good, but breaking, we can direct a PR there. We can also make the min angular version 18 there.

[merto20]

@KevinVandy this is non breaking change. This will allow consumer an option to create a signal component to be rendered. Currently, in order for component to be rendered in the table, it has to use injectFlexRenderContext.

[riccardoperra]

@merto20 My bad 🤦 All of this time I was thinking the FlexRenderComponent were removed in this PR.

Anyway, I may have some suggestion:

for (const prop in this.props) {
      // Only signal based input can be added here
-       if (componentRef.instance?.hasOwnProperty(prop)) {
+       if (componentRef.instance?.hasOwnProperty(prop) && isSignal(componentRef.instance[prop])) { 
        componentRef.setInput(prop, this.props[prop])
      }
    }

I think here, in both renderComponent and renderCustomComponent, we can also invoke isSignal to be sure at least to do it on a potential input. Wdyt? We could move that portion of code in a method in order to reuse it in both places.

Another alternative, but maybe I shouldn't even suggest it, is to use some angular private constants which allows us to set even "non signal" inputs. But potentially this is unsafe.

    const componentDef: ɵComponentDef<unknown> | undefined = Reflect.get(
      componentRef.componentType,
      ɵNG_COMP_DEF // -> this is ɵcmp symbol
    )
    if (!componentDef) {
      // this is not a component
      throw new Error()
    }
    if (componentDef.hasOwnProperty('inputs')) {
      const declaredInputs = Object.keys(componentDef.inputs) // here we have both keys of signal input and decorator-based inputs
    }

[merto20]

@riccardoperra ohh I thought we're discussing the other PR here. Anyway, we can discuss more about the other PR when we need to support signal based directive.

[riccardoperra]

@merto20 Can we add a test to the flexrender.test.ts in order to verify this new behavior?

https://github.com/merto20/angular-table/blob/angular-table--support-for-custom-component-signal/packages/angular-table/src/__tests__/flex-render.test.ts

[merto20]

@merto20 Can we add a test to the flexrender.test.ts in order to verify this new behavior?

https://github.com/merto20/angular-table/blob/angular-table--support-for-custom-component-signal/packages/angular-table/src/__tests__/flex-render.test.ts

@riccardoperra how did you run your test locally? For me, it's always stuck on angular-table:test:lib

[riccardoperra]

@merto20 I mostly use Webstorm interface to run tests

Anyway, running the command manually works fine for me.

Are you running the command from the angular table package?

[KevinVandy]

I might not be able to check out and review this for a few days. Continuing to provide code review from others like @riccardoperra is appreciated.

[merto20]

@riccardoperra I commented the test I created for custom component. Unfortunately, signal inputs are not recognized as component inputs. ComponentRef.setInput will throw exception during the test execution. I suspect it's the test framework we are using. I wanted to try different test framework if my guess is correct, but maybe later when I have time.

[riccardoperra]

We should stay with vitest now. Anyway I think you can do a wrapper component and put in its template your “fake” one passing the data via input binding

[merto20]

We should stay with vitest now. Anyway I think you can do a wrapper component and put in its template your “fake” one passing the data via input binding

I'm not planning to change vitest, just want to confirm if it is because of vitest.
I think having a wrapper will not provide the actual use case on how custom component will be initialized by the flex-renderer. The flex-render directive itself is throwing exception in test. So for now, can we ignore the test?

[riccardoperra]

wrapper component is fine in my opinion, it's a unit test independent by the table itself. Anyway I prefer to avoid commenting test, just use skip method

[riccardoperra]

@KevinVandy this pr should be fine, I think we can merge it

[KevinVandy]

@merto20 I'd like to see the relevant flex render docs get updated along-side this PR

[merto20]

@KevinVandy I created this PR #5590. Pls check. Thanks.