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

RenderComponent - ViewCell Interface Too Restrictive #65

Closed
wabudd1 opened this issue May 26, 2022 · 5 comments
Closed

RenderComponent - ViewCell Interface Too Restrictive #65

wabudd1 opened this issue May 26, 2022 · 5 comments
Assignees
Labels
breaking-change Resolving this issue will introduce a breaking change. enhancement New feature or request
Milestone

Comments

@wabudd1
Copy link

wabudd1 commented May 26, 2022

I'm not sure if there's a specific reason for it to be this way, but the ViewCell interface only allows string | number, but I have a complex object in my data. Ultimately, I'd like to be able to have a RenderComponent that is able to do this.renderedValue = this.value.fullName. It seems as easy as changing the ViewCell.value property to any. Is there a reason not to make this change?

@uap-universe
Copy link
Collaborator

uap-universe commented May 30, 2022

One of the goals is to remove any where possible to become more type-safe.

That said, the custom renderer stuff is way more fucked up as you might think. Because the truth is, that the renderer receives the object returned by the valuePrepareFunction no matter what the type is.

This is because typescript is not type-safe. You can actually assign arbitrary objects to value: string|number and they do not need to be string or number. You just need to make it so the compiler does not see it.

So you can - without regrets - just implement your renderer like this

// assume your row data looks like this
export class MyComplexObject {
   attr: MyComplexAttribute;
   // ...
}

export class MyComplexAttribute {
   fullName: string;
   // ...
}

export class MyRenderComponent implements OnInit, ViewCell {

    // @ts-ignore
    @Input() value: MyComplexAttribute | string;
    @Input() rowData!: MyComplexObject;

    data!: MyComplexAttribute;

    ngOnInit(): void {
        this.data = this.value as MyComplexAttribute;
        this.value = this.data.fullName; // for example - but you don't need this,
                                                          // you can directly use the stuff in the HTML template

The default valuePrepareFunction returns the string representation of your attribute. So you need a custom one, just returning your attribute.

settings = {
  // ...
  columns: {
    // ...
    attr: {
      // ...
      valuePrepareFunction: (value, row, cell) => value
   }
  }
}

The same does not only work with value. You can also return row or cell from the valuePrepareFunction and then this is accessible in your renderer.

Of course it would be nice to have a decent type for ViewCell.value and I can understand that, given the circumstances, it might look like any is actually a correct choice (because the thing can obviously be literally anything).

Currently the correct type would be T | Property | Cell<T, Property> | Row<T> where T is the type of the object (MyComplexObject here) and Property is the type of the property rendered in this column (MyComplexAttribute here).

Changing that in a reasonable way is not so easy. I keep this ticket open as a reminder and tag it with enhancement.

@uap-universe uap-universe added the enhancement New feature or request label May 30, 2022
@wabudd1
Copy link
Author

wabudd1 commented Jun 2, 2022

Excellent, thank you for taking the time to do that writeup. It makes a lot of sense.

@uap-universe
Copy link
Collaborator

Maybe somehow remotely related with #115 which suffers from similar type confusion problems. But maybe there the possibility for a non-breaking small step cleanup for v2.9.0.

@uap-universe uap-universe added the breaking-change Resolving this issue will introduce a breaking change. label Apr 17, 2023
@uap-universe uap-universe removed this from the v2.9.0 milestone Apr 17, 2023
@uap-universe
Copy link
Collaborator

Unfortunately, any sort of type change would be likely a (minor) breaking change for the users who already use the strict typing. Will reconsider this for the next major update 3.0.0.

@uap-universe uap-universe added this to the v3.0.0 milestone Apr 17, 2023
@uap-universe
Copy link
Collaborator

Actually, we can get rid of the ViewCell interface entirely. There is no reason why the componentInit function shouldn't be sufficient to initialize the component with the cells value. This simplifies things a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Resolving this issue will introduce a breaking change. enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants