Unregistered has() features fail silently...

[kitsonk]

@kitsonk commented on Tue Jun 21 2016

Three challenges with the has() API:

• If a feature is not registered, it just fails silently returning undefined. This leads to potential logic errors, assuming a feature is being properly detected when in fact it isn't. We should throw.
• Features are case sensitive, and while by convention, we keep labels all lower case, consumers of the API may unintentionally make mistakes... if we coerced the features to lowercase, it would avoid mistakes and errors.
• When add is passed with override = false it simply returns a false if the add fails because it is already present. The way we currently consume the API, we don't handle those failures and it is likely that those downstream using the API will do the same. We should consider throwing when the override fails, as it is clearly again something someone should be explicit about, instead of unintentional behaviour.

---

@matt-gadd commented on Wed Jun 22 2016

• this really does seem like a trap given you can have a test result of undefined, and also in the majority of cases the user has likely written a loose truthy/falsey if anyway. I think we should definitely throw, especially given we have an exists api anyway for what would be the only useful side effect of returning undefined currently.

• although agreed it probably would make things safer, it does seem a bit opinion'y. would you co-erce to lowercase on both the set (adding) and getting? ie: has.add('aBc', true) has('ABC')

  are equivalent?

• I don't have a problem with the current behaviour of add because the return values true (added) and false (not added) aren't overloaded like the first bullet point. It seems like we added the overwrite and no-overwrite paths for a reason, so I don't think i'd expect to get an error here?

---

@kitsonk commented on Wed Jun 22 2016

it does seem a bit opinion'y.

While I am on border on this upon further reflection, I found some issues when breaking out the shims where the feature was added as abc but was tested as abC. If we throw on missing features, then I think largely it negates this and while we might have our opinion about the feature naming conventions, we wouldn't have to enforce that on others.

It seems like we added the overwrite and no-overwrite paths for a reason, so I don't think i'd expect to get an error here?

But we have an exists() API... so in theory, if you were in doubt of it being registered. With that available, I cannot see the use case of just performing an add() without override "just in case"... clearly that is an error in the code and it would be better to throw, so that the developer addresses the logic error and doesn't depend upon things they think they have accomplished.

---

@matt-gadd commented on Wed Jun 22 2016

While I am on border on this upon further reflection, I found some issues when breaking out the shims where the feature was added as abc but was tested as abC. If we throw on missing features, then I think largely it negates this and while we might have our opinion about the feature naming conventions, we wouldn't have to enforce that on others.

the fact we've actually managed to fail at that in our own code does give more credence to the argument :).

But we have an exists() API... so in theory, if you were in doubt of it being registered. With that available, I cannot see the use case of just performing an add() without override "just in case"... clearly that is an error in the code and it would be better to throw, so that the developer addresses the logic error and doesn't depend upon things they think they have accomplished.

you could argue that the add function is already doing too much by having different behaviour based on a flag in the first place, and we should just pick the most common one as the api (both scenarios are possible via the exists api anyway).

[kitsonk]

We should settle this once and for all before the beta.

[rishson]

My 2p: 1) we should throw on undefined 2) we should throw on mis-spells, but we should coerce to lowercase and compare. If a misspell, we should throw a did you mean abC error 3) don't know enough about has and exists to have an opinion yet.

[rorticus]

I guess I won't do 1)...

[kitsonk]

You even have the blame on! :-D

If there is really nothing you think we can improve upon at the moment, feel free to close it.

[dylans]

has case sensitivity was addressed. Leaving open for now in case there's something else to consider. Feel free to close if we're done here.