Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
shavinac opened this issue Jul 12, 2022 · 1 comment
Open

Comments

@shavinac
Copy link
Contributor

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
Copy link
Contributor Author

Related to #8 for the @gitcoinco/passport-sdk-writer package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant