feat: add get user subscription api call#411
Conversation
packages/snjs/lib/application.ts
Outdated
| if (!response.error && response.data) { | ||
| return (response as GetSubscriptionResponse).data!.subscription | ||
| } | ||
| } catch (e) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
We want the errors to propagate:
if(response.error != null) throw new Error(error.message);
There was a problem hiding this comment.
And no need to wrap in try/catch. The error that happens in await will automatically be embedded in the error response part of the Promise.
packages/snjs/lib/application.ts
Outdated
| try { | ||
| const response = await this.apiService.getAvailableSubscriptions(); | ||
| if (!response.error && response.data) { | ||
| return (response as GetAvailableSubscriptionsResponse).data! | ||
| } | ||
| } catch (e) { | ||
| return undefined; | ||
| } | ||
| } |
| authentication: this.session?.authorizationValue, | ||
| fallbackErrorMessage: messages.API_MESSAGE_FAILED_SUBSCRIPTION_INFO, | ||
| }); | ||
| this.processResponse(response); |
There was a problem hiding this comment.
I think processResponse already happens in this.request.
There was a problem hiding this comment.
Ah good catch, thanks! 🙂
gorjan5sk
left a comment
gorjan5sk
left a comment
There was a problem hiding this comment.
Approved! :) One small comment regarding processResponse.
| public async getUserSubscription(): Promise<Subscription | undefined> { | ||
| const response = await this.sessionManager.getSubscription(); | ||
| if (response.error) { | ||
| throw new Error(response.error.message) |
There was a problem hiding this comment.
Hmm why are we throwing? We should propagate this to the client so the user can take action. Otherwise this might block execution of other essential tasks.
There was a problem hiding this comment.
When throwing an error in an async function it triggers the catch/error of the Promise. That's how it's propagated towards the client.
|
@mobitar the failing e2e test here also failed in your latest commit in the master branch. Safe to ignore? |
|
Yep need to look into it. Is a test/deinit issue rather than core issue. |
5 participants
https://app.asana.com/0/1199914526147392/1200845194469719/f