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

Combination of HostBinding and signals is not supported (input signals too) #53888

Open
Celtian opened this issue Jan 11, 2024 · 24 comments
Open
Assignees
Labels
area: core Issues related to the framework runtime core: host and host bindings cross-cutting: signals needs: discussion Indicates than an issue is an open discussion
Milestone

Comments

@Celtian
Copy link

Celtian commented Jan 11, 2024

Which @angular/* package(s) are relevant/related to the feature request?

core

Description

I am currentely trying to use new input signal api but I am unable to replace this code with DRY.

  @HostBinding('attr.title')
  @Input({ required: true, alias: 'appModalClose' })
  public closeBtnText!: string;

Proposed solution

Maybe it should be replaced by something like

public closeBtnText = input.required<string>({ alias: 'appModalClose', hostBinding: ['attr.title'] });

Alternatives considered

I was trying to find some solutions, but this is not working

@HostBinding('attr.title')
public closeBtnText = input.required<string>({ alias: 'appModalClose' });
  1. Host is stacked in memory leak
@Component({
  // eslint-disable-next-line @angular-eslint/no-host-metadata-property
  host: {
    '[title]': 'closeBtnText()',
  },
})
export class ModalCloseComponent {
  public closeBtnText = input.required<string>({ alias: 'appModalClose' });
}
@devversion devversion self-assigned this Jan 11, 2024
@devversion
Copy link
Member

My initial thought is that @HostBinding is expecting raw values. A Signal is not supported, even without signal inputs. In the host field of the component the signal can be read and used to e.g. bind attr.title. I will look more into this

@alxhub
Copy link
Member

alxhub commented Jan 11, 2024

It's also possible to split the two, and write:

appModalClose = input.required<string>();

@HostBinding('attr.title')
get title() { return this.appModalClose(); }

@devversion
Copy link
Member

devversion commented Jan 12, 2024

Thanks again for this issue. I've chatted a little with @pkozlowski-opensource and we both felt like supporting @HostBinding with signals is not necessarily something we would want to support now (without more evidence that there are requiring use-cases). Mostly because:

  • We likely want to avoid encouraging two patterns. hosts and @HostBinding achieve the same thing.
  • There are still ways to use @HostBinding, as Alex pointed out.
  • Combining a decorator (@HostBinding) with signal inputs (that are no longer decorator-based) feels confusing.
  • Calling signals directly in the host field of e.g. @Directive is a more natural/obvious consumption in the reactive context. I.e. it matches with the mental model that "reading signals" is necessary so that the directive/component will update whenever those change.

You mentioned something with memory leak when using host. Can you share more details please? Below seems to work as expected:

@Directive({
  standalone: true,
  selector: 'my-dirx',
  host: { '[attr.title]': 'bla()' },
})
export class MyDir {
  bla = input.required<string>();
}

@Celtian
Copy link
Author

Celtian commented Jan 13, 2024

@devversion I have doubble checked

@Directive({
  standalone: true,
  selector: 'my-dirx',
  host: { '[attr.title]': 'bla()' },
})
export class MyDir {
  bla = input.required<string>();
}

and it is working right now

@devversion devversion changed the title Combination of HostBinding & new input signal api is not probably supported Combination of HostBinding and singals is not supported (input signals too) Jan 14, 2024
@devversion devversion changed the title Combination of HostBinding and singals is not supported (input signals too) Combination of HostBinding and signals is not supported (input signals too) Jan 14, 2024
@dylhunn dylhunn added the area: core Issues related to the framework runtime label Jan 17, 2024
@ngbot ngbot bot added this to the Backlog milestone Jan 17, 2024
@cyraid
Copy link
Contributor

cyraid commented Jan 30, 2024

Oh, so we're stuck with using a string expression with no IDE support? That's unfortunate.
Any plans to support Signal in a HostBinding?

@dkimmich-onventis
Copy link

@cyraid Which IDE do you use? I am using WebStorm, and it works like a charm, with code highlighting, suggestions and everything.

image

@cyraid
Copy link
Contributor

cyraid commented Feb 29, 2024

@dkimmich-onventis I use vscode. It would be nice if angular highlighted and syntax/error checked the host field in vscode. :)

@adammolnar
Copy link

Why is the syntax

host: {
  '[class.foo]': 'bar()', 
},

instead of

host: {
  '[class.foo]': bar(), 
},

which would make more sense to me. In other words: why is the value a string? Just wondering.

@dkimmich-onventis
Copy link

dkimmich-onventis commented Mar 5, 2024

Because bar in this context is not defined. this.bar is also not defined, as we are inside the decorator and not inside the class. It needs to be a string so TypeScript doesn't throw errors.

@JeanMeche
Copy link
Member

There is no reference named bar in the scope the decorator.

@nickdobson
Copy link

If this is going to be the recommended approach going forward, should this style guide rule be updated?

https://angular.io/guide/styleguide#style-06-03

@devversion
Copy link
Member

@nickdobson Yeah, we likely need to discuss this more with the team. Good point.

@c-harding
Copy link

c-harding commented Mar 26, 2024

@devversion has a function in the constructor been considered?

constructor() {
  hostBinding('class.hidden', () => this.isHidden());
  hostBinding('class.hidden', this.isHidden); // alternative: isHidden is a signal
}

@k3nsei
Copy link

k3nsei commented Apr 9, 2024

I like having hostBinding and hostListener functions more, than using not recommended by style guide host property of directive decorator.

@JeanMeche
Copy link
Member

@k3nsei As mentionned in #54284, the style guide is know to be a bit outdated on some topics.

As mentionned here, the host property is being recommended over the usage of the hostBinding/hostListener.

@k3nsei
Copy link

k3nsei commented Apr 9, 2024

@JeanMeche but HostBinding and HostListener are working with class inheritance. We are using that in our FormControl abstractions to not repeat code.That means regression in some way, because class decorators are not inherited.

We mostly need to do that, to not copy/paste code to use custom form controls inside angular material form field, as it's requires a lot of stuff to be implemented in custom form control.

We could of course use some mix of hostDirectives and base abstract class which delegates methods and getters and setters calls to this directives basically works as a proxy. But this sounds like a hack to me.

@shajz
Copy link

shajz commented Apr 16, 2024

@k3nsei As mentionned in #54284, the style guide is know to be a bit outdated on some topics.

As mentionned here, the host property is being recommended over the usage of the hostBinding/hostListener.

@JeanMeche I've noticted the recommendation on the angular.dev website is conflicting with the angular.io styleguide, as it stills recommends HostListener/HostBinding over the host property : https://angular.io/guide/styleguide#style-06-03.

I was wondering why the @angular-eslint/no-host-metadata-property rule disagreed with your statement 😅. It's because it points specifically to the current styleguide https://github.com/angular-eslint/angular-eslint/blob/main/packages/eslint-plugin/docs/rules/no-host-metadata-property.md.

Should the styleguide be updated?

@k3nsei
Copy link

k3nsei commented Apr 16, 2024

@shajz angular-eslint is community driven package, thats why there could be some divergences. You can always create issue in their repository or even PR to align with latest recommendations.

@shajz
Copy link

shajz commented Apr 16, 2024

Thanks for pointing it out but that was not my main point, merely a nudge in understanding the issue. Sorry if my message was misunderstood:

The problem I highlighted is that both the official Angular websites conflict on the recommendation of using host vs HostBinding/HostListener. That should be addressed on the angular repo :)

@JeanMeche
Copy link
Member

The style guide will be updated as part of the upcoming v18 release. This is tracked by ##54284.

@k3nsei
Copy link

k3nsei commented Apr 16, 2024

@JeanMeche could this release also be synchronized with folks from angular-eslint as it's used by most projects using Angular?

@JeanMeche
Copy link
Member

@k3nsei angular-eslint/angular-eslint#1772 I've suggested the change, thank you.

@Kordrad
Copy link

Kordrad commented May 21, 2024

I like this suggestion

public closeBtnText = input.required<string>({ alias: 'appModalClose', hostBinding: ['attr.title'] });

or event something like this

@Component({/*...*/})
export class Component {
    readonly customInput = input(false);
    readonly customBinding = hostBinding('attr.custom', () => this.customInput());
}

For other solutions, this also sounds good

@k3nsei angular-eslint/angular-eslint#1772 I've suggested the change, thank you.

@lkster
Copy link

lkster commented May 24, 2024

@alxhub just wondering - is no-decorator signal-way approach even considered or you just haven't touched it yet? I'm about the way @c-harding provided:

constructor() {
  hostBinding('class.hidden', () => this.isHidden());
  hostBinding('class.hidden', this.isHidden); // alternative: isHidden is a signal
}

I'm curious about potential disadvantages of such approach if you already discussed it. For me it would make a great fit with effect instead of having one thing in constructor and another in component's metadata but I suppose this can be something that you just plan to tackle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to the framework runtime core: host and host bindings cross-cutting: signals needs: discussion Indicates than an issue is an open discussion
Projects
None yet
Development

No branches or pull requests