openid / OpenYOLO-Web

Web protocol for credential exchange and update - "You Only Login Once"
http://openid.net/wg/ac/
Apache License 2.0
100 stars 16 forks source link

Implemented new errors design #39

Closed TMSCH closed 7 years ago

TMSCH commented 7 years ago

The new error codes have been added, and only OpenYoloError are now exposed and sent to the client side of the library.

Also renamed all variables prefixed by OY to use OpenYolo as prefix for consistency.

codecov-io commented 7 years ago

Codecov Report

Merging #39 into master will decrease coverage by 0.19%. The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #39     +/-   ##
=========================================
- Coverage   87.44%   87.25%   -0.2%     
=========================================
  Files          34       34             
  Lines        1410     1428     +18     
  Branches      167      168      +1     
=========================================
+ Hits         1233     1246     +13     
- Misses        177      182      +5
Impacted Files Coverage Δ
ts/protocol/preload_request.ts 100% <ø> (ø) :arrow_up:
ts/protocol/data.ts 100% <ø> (ø) :arrow_up:
ts/api/credential_request.ts 100% <100%> (ø) :arrow_up:
ts/protocol/rpc_messages.ts 95.31% <100%> (ø) :arrow_up:
ts/api/credential_save.ts 100% <100%> (ø) :arrow_up:
ts/protocol/post_messages.ts 100% <100%> (ø) :arrow_up:
ts/api/hint_available_request.ts 100% <100%> (ø) :arrow_up:
ts/api/proxy_login.ts 100% <100%> (ø) :arrow_up:
ts/api/hint_request.ts 100% <100%> (ø) :arrow_up:
ts/api/api.ts 69.84% <56.25%> (ø) :arrow_up:
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7195c8b...61aff4d. Read the comment docs.

iainmcgin commented 7 years ago

One general query: is it necessary to prefix types with "OpenYolo" when everything is namespaced by modules, and can be renamed by importers?

TMSCH commented 7 years ago

When I migrated to 2.4, some errors started to be raised due to Weak type detection and the fact that we would declare interfaces with the same name than ambient interfaces (Credential, CredentialRequestOptions). To avoid these issues I had to prefix them (originally, OY, changed to OpenYolo for consistency). For Errors, OpenYoloError is better IMO than ExposedError. However, I could remove the OpenYolo prefix from internal errors/interfaces. What do you think?

iainmcgin commented 7 years ago

Ah, so it's clashing with the type definitions from the credential management API? That's unfortunate. I'd prefer to avoid the prefixing type names wherever possible, and deal with the ambiguity by renaming the imports where necessary. Will that work?

TMSCH commented 7 years ago

Yes, I can also rename all the imports declarations that clash with the Credentials Management API. However it's pretty error prone as I discovered recently. For instance, one of the class that was using our CredentialRequestOptions was not importing it from protocol/data, but it would still work fine, and the tests would pass for this file. I discovered this issue after quite some time debugging an error that was in a completely different file... So I understand prefixing is not great, but using the exact same name in our code is not great neither. I am not a huge fan of the prefixing neither, but at least, it avoids such issues. Let me know what you think.

(it only holds for clashing declarations, and potentially OpenYoloError that cannot be stripped from OpenYolo)