passport-sdk icon indicating copy to clipboard operation
passport-sdk copied to clipboard

[reader] use clearer return types for getPassport and related methods

Open shavinac opened this issue 1 year ago • 1 comments

https://github.com/gitcoinco/passport-sdk/blob/46a34846402d1690f81602ed265504265beb59ae/packages/reader/src/reader.ts#L46-L49

Currently getPassport has a return type of Promise<CeramicPassport | Passport | false>, where false means "no passport". This is unintuitive - usually you would expect undefined or null value returned if the requested data was not found. Also it is confusing that both CeramicPassport | Passport are both valid return types.

Since getPassport returns a Promise, we could also use Promise.reject() to handle any error cases.

For example, getPassportStream wraps all Ceramic requests in a try/catch, which swallows any network related errors, and returns false in those cases. This could lead to incorrect behavior such as creating a new passport, even though there may be an existing passport but it failed to load due to network errors etc. https://github.com/gitcoinco/passport-sdk/blob/46a34846402d1690f81602ed265504265beb59ae/packages/reader/src/reader.ts#L60-L63

shavinac avatar Jul 12 '22 19:07 shavinac