angular_analyzer_plugin icon indicating copy to clipboard operation
angular_analyzer_plugin copied to clipboard

hint about binding for optional @Input()

Open zoechi opened this issue 8 years ago • 8 comments

image

@Directive(
  selector: '[virtualScroll]',
)
class VirtualScrollDirective  {
  bool _active;
  @Input()
  set virtualScroll(bool active) {
    if (active == _active) {
      return;
    }

    _active = active;
    if (_active) {
      _restoreState();
    } else {
      _persistState();
    }
  }

I think it shouldn't cause a hint/warning

zoechi avatar Jun 19 '17 14:06 zoechi

We'll add a fix for this shortly. We're still treating directive bindings input [virtualScroll] and virtualScroll as the same when they shouldn't be in the case where input and directive share the same name space.

mk13 avatar Jun 19 '17 20:06 mk13

This error is correct. You get empty string when using this binding like you showed, not a boolean. If you check the angular_components lib, you'll see they accept dynamic in the setter and treat '' as true. You can do the same to get the error to go away.

This really is more of an angular design bug; its an unexpected behavior, and the workaround is not perfect either as type checking is turned off for the input and [mybool]="''" counterintuitively enables the flag.

MichaelRFairhurst avatar Jun 19 '17 20:06 MichaelRFairhurst

@MichaelRFairhurst I see. I didn't interpret the hint as type violation. If bool had been mentioned in the message, it would be more clear. Is there a way to improve the message. (instead of "is not a string" use "is of type bool")

zoechi avatar Jun 19 '17 20:06 zoechi

EDIT: Discussed with Mike - and I was confusing what was going on with the error flag.

Is there a way to improve the message. (instead of "is not a string" use "is of type bool") We can't really add a more detailed message because the error message is 100% correct. Anything that doesn't utilize a bracket-form syntax for an input binding is automatically casted as a String value.

If you're using a someInput(bool x), and utilizing <div directive1 someInput>, then this will automatically try and parse a string into someInput binding. In the case that you want someInput to exist without a bracket, you'll have to add the following form:

@Input()
set virtualScroll(String flag) {
   // Properly cast `flag` as a bool
}

However, the above implementation means that the bracket syntax won't work either: <div directive1 [virtualScroll]="true">, but instead you'll have to use <div directive1 [virtualScroll]="'true'">.

In the case where you want all formats supported: <div directive1 virtualScroll> <div directive1 virtualScroll="true"> <div directive1 [virtualScroll]="true">,

You'll have to make the input type dynamic:

@Input()
set virtualScroll(dynamic flag) {
    // Check if flag is string or bool, then do proper casting-work to make it work
}

Unfortunately though, now we can't properly support dynamic types since our analysis works on static definitions. So something like: <div directive1 [virtualScroll]="1234"> won't be properly flagged within the IDE as being incorrect since it can accept anything at this point and you'll have to resort to run-time errors.

mk13 avatar Jun 19 '17 21:06 mk13

What about

"using the attribute virtualScroll without brackets binds the attribute value "", a String, to the input virtualScroll on VirtualScrollDirective, which is expected to be of type String, but is actually of type bool."

Edit: This is just really weird behavior that's really hard to describe. The current message explained everything just as well, I think, to be totally honest. But its so long people skim over it. And its so detailed/dense that people don't get it unless they really carefully read it. I'm not sure any error message can make this clear without having the same set of problems. Its also tricky, because in this case the error was with how the component was defined, but we can't raise an error there. We can only raise an error where its used, at which point its a degree removed. Maybe we can have the error say, "check your directive is correctly defined"?

MichaelRFairhurst avatar Jun 19 '17 21:06 MichaelRFairhurst

Perhaps it's just an issue because I had so many false hints and warnings in the past because of the early stage of the plugin. When I start trusting the hints more (recent updates got rid of most false hints already), I'll try harder to understand the messages before creating an issue and complaining about it ;-) I see it's not too easy to fix/improve because of the somewhat confusing behavior of Angular.

@MichaelRFairhurst I think this is a good suggestion

"using the attribute virtualScroll without brackets binds the attribute value ""

because it's not obvious that there is a binding happening at all, because the intention was just to get the directive added (the bound value is for special cases), and I think thats the root cause for the confusion.

zoechi avatar Jun 20 '17 07:06 zoechi

Perhaps it's just an issue because I had so many false hints and warnings in the past because of the early stage of the plugin. When I start trusting the hints more (recent updates got rid of most false hints already), I'll try harder to understand the messages before creating an issue and complaining about it ;-)

In this stage of development, I think Mike and I would prefer it if you reported every suspicious issue you see :) This is just the rare case where the usage is not entirely clear either (it took us a lot of communication with the Angular team to fully figure out what was going on too)- but hopefully an error message with more clarity would help.

mk13 avatar Jun 20 '17 17:06 mk13

I think Mike and I would prefer it if you reported every suspicious issue

@mk13 I have reported all issues I encountered so far. Until a few days ago I had dozens of false hints I couldn't get rid of, but these are now fixed since the last update a few days ago. So now a new era begins :-) This one is a new one that I encountered while adding new code.

zoechi avatar Jun 20 '17 21:06 zoechi