{"action":"created","issue":{"url":"https://api.github.com/repos/dojo/has/issues/18","repository_url":"https://api.github.com/repos/dojo/has","labels_url":"https://api.github.com/repos/dojo/has/issues/18/labels{/name}","comments_url":"https://api.github.com/repos/dojo/has/issues/18/comments","events_url":"https://api.github.com/repos/dojo/has/issues/18/events","html_url":"https://github.com/dojo/has/issues/18","id":180993514,"number":18,"title":"Unregistered has() features fail silently...","user":{"login":"kitsonk","id":1282577,"avatar_url":"https://avatars.githubusercontent.com/u/1282577?v=3","gravatar_id":"","url":"https://api.github.com/users/kitsonk","html_url":"https://github.com/kitsonk","followers_url":"https://api.github.com/users/kitsonk/followers","following_url":"https://api.github.com/users/kitsonk/following{/other_user}","gists_url":"https://api.github.com/users/kitsonk/gists{/gist_id}","starred_url":"https://api.github.com/users/kitsonk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kitsonk/subscriptions","organizations_url":"https://api.github.com/users/kitsonk/orgs","repos_url":"https://api.github.com/users/kitsonk/repos","events_url":"https://api.github.com/users/kitsonk/events{/privacy}","received_events_url":"https://api.github.com/users/kitsonk/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dojo/has/labels/beta-blocker","name":"beta-blocker","color":"ba6300"},{"url":"https://api.github.com/repos/dojo/has/labels/discussion","name":"discussion","color":"0052cc"},{"url":"https://api.github.com/repos/dojo/has/labels/one-hit-wonder","name":"one-hit-wonder","color":"184931"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-10-04T20:16:09Z","updated_at":"2016-10-04T20:17:03Z","closed_at":null,"body":"@kitsonk commented on [Tue Jun 21 2016](https://github.com/dojo/core/issues/172)\n\nThree challenges with the `has()` API:\r\n\r\n* 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.\r\n* 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.\r\n* 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.\r\n\n\n---\n\n@matt-gadd commented on [Wed Jun 22 2016](https://github.com/dojo/core/issues/172#issuecomment-227681076)\n\n* 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.\r\n\r\n* 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:\r\n`has.add('aBc', true)`\r\n`has('ABC')`\r\n\r\n    are equivalent?\r\n\r\n* 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?\n\n---\n\n@kitsonk commented on [Wed Jun 22 2016](https://github.com/dojo/core/issues/172#issuecomment-227686270)\n\n> it does seem a bit opinion'y.\r\n\r\nWhile 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.\r\n\r\n> 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?\r\n\r\nBut 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.\n\n---\n\n@matt-gadd commented on [Wed Jun 22 2016](https://github.com/dojo/core/issues/172#issuecomment-227690906)\n\n>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.\r\n\r\nthe fact we've actually managed to fail at that in our own code does give more credence to the argument :).\r\n\r\n>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.\r\n\r\nyou 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). \r\n\n\n"},"comment":{"url":"https://api.github.com/repos/dojo/has/issues/comments/251500259","html_url":"https://github.com/dojo/has/issues/18#issuecomment-251500259","issue_url":"https://api.github.com/repos/dojo/has/issues/18","id":251500259,"user":{"login":"kitsonk","id":1282577,"avatar_url":"https://avatars.githubusercontent.com/u/1282577?v=3","gravatar_id":"","url":"https://api.github.com/users/kitsonk","html_url":"https://github.com/kitsonk","followers_url":"https://api.github.com/users/kitsonk/followers","following_url":"https://api.github.com/users/kitsonk/following{/other_user}","gists_url":"https://api.github.com/users/kitsonk/gists{/gist_id}","starred_url":"https://api.github.com/users/kitsonk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kitsonk/subscriptions","organizations_url":"https://api.github.com/users/kitsonk/orgs","repos_url":"https://api.github.com/users/kitsonk/repos","events_url":"https://api.github.com/users/kitsonk/events{/privacy}","received_events_url":"https://api.github.com/users/kitsonk/received_events","type":"User","site_admin":false},"created_at":"2016-10-04T20:17:03Z","updated_at":"2016-10-04T20:17:03Z","body":"We should settle this once and for all before the beta."}}
{"actor":{"display_login":"kitsonk"}}