Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Introduce a way for a component to require an attribute #16126

Closed
1 of 3 tasks
ZitaNemeckova opened this issue Jul 24, 2017 · 12 comments
Closed
1 of 3 tasks

Introduce a way for a component to require an attribute #16126

ZitaNemeckova opened this issue Jul 24, 2017 · 12 comments

Comments

@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Jul 24, 2017

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Problem description:

Let's have a component (for simpleButton) with following bindings:

bindings: {
    name: '@',
    enabled: '<',
    onClick: '&',
  },

And it needs to be used with all the bindings set like this <simple-button enabled="true" name="Foo" onClick="foo()"></simple-button>. A programmer shouldn't be able to use it without one or more non-optional attribute like this <simple-button></simple-button> or <simple-button enabled="true" name="Foo"></simple-button>.

There's no way to ensure that all required bindings will be set. This causes unpredictable behavior and it's a bit hard to debug if any of them is forgotten.

https://plnkr.co/edit/OwrnxuU3QhsvyNBMSrXh?p=preview

Current behavior:

There's no way to enforce that a binding in the component is required and must be set as an attribute.

If a binding is non-optional (without ?) and no value is given it's set to undefined.

At the moment <simple-button enabled="undefined"></simple-button> and <simple-button></simple-button> are indistinguishable from each other (when non-optional).

Expected / new behavior:

It throws an error if required binding is not set as an attribute of a component.

Angular version:

latest

Anything else:

I see two possible ways to solve this.
The first is to enforce all bindings without ? to be set as an attribute of a component. But I can see that may be a bit too radical. I tried and it worked but broke 105 tests :/
The second is to introduce a new marker that will indicate that attribute must be set. ! looks like a good candidate.

I'm willing to submit a PR if there's a decision how to proceed :)

@frederikprijck
Copy link
Contributor

frederikprijck commented Jul 24, 2017

For what it's worth: Angular doesn't have this option neither (unless I'm missing something). Even tho I like the idea, I'm not sure if it's a good idea to introduce something that's going to make upgrading only harder.

I also remember this has been discussed before but I can't seem to remember which issue/pr that was or what the outcome was. Maybe @gkalpak or @Narretz can shed their light ?

@himdel
Copy link

himdel commented Jul 24, 2017

If it helps, Vue seems to have a concept of required props, which seems close https://vuejs.org/v2/guide/components.html#Prop-Validation .

In Angular (2+), it would be easy to create a @Required decorator using elementRef.nativeElement - since you decorate each attribute, it could just look for that name in nativeElement's props.

In AngularJS (1) I see no nice way of achieving that... (apart from accessing angular internals from the controller to read the parsed bindings and comparing that to $attrs)

@Narretz
Copy link
Contributor

Narretz commented Jul 24, 2017

Technically, props in AngularJS that aren't marked as optional are required, but an absent attribute doesn't trigger a warning or an error. The reason why this hasn't been added is that it would possibly break many apps, because not every directive author has used optional bindings even though they should. So a change at this point could be quite disruptive for questionable gain. We could introduce error throwing for missing required attributes behind a flag though,.

See also here #6064

An updated PR that addressed #6064 (comment) could be considered, whether directly or behind a flag.

@himdel
Copy link

himdel commented Jul 24, 2017

@Narretz thanks.. I understand this has historical reasons, but.. if we introduced a new modifier to mean strictly required, that wouldn't break anybody, right?

(At least, I assume that ! is not a valid part of an attribute name, so =! would not clash with = +
!weird_name_startign_with_an_exclamation_mark)

@jeserkin
Copy link

jeserkin commented Jul 24, 2017

@Narretz could it be done through new $compileProvider option or through same .debugInfoEnabled(), that already exists? I would assume, that in production it shouldn't break anything, since in production .debugInfoEnabled() shouldn't be set to true. And if it brakes in development environment do not see any issue with that. Would additional bullet in Breaking changes section in CHANGELOG.md if it even would be considered as breaking change.

@Narretz
Copy link
Contributor

Narretz commented Jul 24, 2017

@jeserkin I don't see this strictly as debug info. Debug Info data should not be relied upon in production, but required attributes is something you should be able to rely upon. A new compileProvider option sounds better.

@himdel I'd rather not introduce a new modifier for a behavior that is technically already there. ! would be "strict requirement" and I think this is better handled by introducing a flag on the compileProvider or making throwing the default - which would benefit all directives, not only those that use !. The Directive Defintion object is complicated enough.

@himdel
Copy link

himdel commented Jul 24, 2017

Understood...the debugInfo idea would work for us, even just seeing a warning when that happens, doesn't necessarily have to be an exception.

All we really need is to clue the developers that they have forgotten something :). (Or that the component got updated since and is misssing something now.)

@jeserkin
Copy link

jeserkin commented Jul 24, 2017

@Narretz does something like $compileProvider.strictComponentBindingsEnabled(true/false) sound eligible for implementation and PR?

@ZitaNemeckova
Copy link
Contributor Author

@jeserkin Sounds good to me :)

@jeserkin
Copy link

@ZitaNemeckova as I understood, you gonna do a PR to add this feature, correct? Or you've only created and issue?

@ZitaNemeckova
Copy link
Contributor Author

@jeserkin I will make a PR :) Thank you for finding nice way to solve our issue :)

@Narretz
Copy link
Contributor

Narretz commented Jul 28, 2017

There's now a config option that makes directives throw when non optional bindings are missing. f1d01bb

@Narretz Narretz closed this as completed Jul 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants