feat: expose subscriptions limited availability#841
Conversation
| if (query.kind === 'Document') { | ||
| return print(query); | ||
| } | ||
| throw new Error('queryToString: query must be a string or a DocumentNode'); |
There was a problem hiding this comment.
The previous checks were exhaustive.
There was a problem hiding this comment.
question: shouldn't we keep in mind consumers who may not use typescript? Especially if they try to pass a query that can be handled by print, but is not a DocumentNode, the results can be confusing
I'd keep the old checks, but maybe adjust the message, we don't need to include the function name
There was a problem hiding this comment.
I'll add the checks back as we're pressed for time.
Not gonna look into the invalid query or Javascript questions atm (were there before), but could be issues yup. Good thinking!
| shareReplay(1), | ||
| ); | ||
|
|
||
| this.retryableErroredSubscriptionsMessageIds$.subscribe(); |
There was a problem hiding this comment.
Actually considering removing this. But will tackle with rest of changes.
There was a problem hiding this comment.
Nah, we need it as it's being used in retry logic.
KonradPaluch
left a comment
KonradPaluch
left a comment
There was a problem hiding this comment.
looks good
two blocking comments
- removeEventListener handler reference
- type safety in parseQuery
| messageId: message.id, | ||
| }); | ||
|
|
||
| this.messagesSubscribers.get(message.id)?.next(message.payload.data); |
There was a problem hiding this comment.
suggestion: maybe message.payload?.data in case an empty message arrives?
| if (query.kind === 'Document') { | ||
| return print(query); | ||
| } | ||
| throw new Error('queryToString: query must be a string or a DocumentNode'); |
There was a problem hiding this comment.
question: shouldn't we keep in mind consumers who may not use typescript? Especially if they try to pass a query that can be handled by print, but is not a DocumentNode, the results can be confusing
I'd keep the old checks, but maybe adjust the message, we don't need to include the function name
| if (typeof window !== 'undefined') { | ||
| window.removeEventListener('offline', this.sendPingWithThisBound); | ||
| window.addEventListener('offline', this.sendPingWithThisBound); | ||
| window.removeEventListener('offline', () => { |
There was a problem hiding this comment.
issue: we need to pass the same function to remove listener as we did to add listener. An arrow function is always a new one
2 participants
Let's consumers handle state where some subscriptions fail.
Checklist