-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: improve invite already accepted handling #506
Conversation
ed30510
to
d5b9fd0
Compare
Pull Request Test Coverage Report for Build 7350645555
💛 - Coveralls |
} | ||
|
||
if (typeof response !== 'object') { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return a true or false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.
if ( | ||
'error' in response && | ||
typeof response.error === 'string' && | ||
response.error.includes('already accepted') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the API be modified to have a consistent behavior instead of two ways to return the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdlavin Yes, it definitely should. I'd like to consider that change out-of-scope for this PR.
FWIW, as far as I can tell, the behavior is consistent. But, the current SDK logic is simply not correct. I'd like to get this sub-optimal extra handling in place, then follow-up with a deeper investigation to simplify the logic (client-side, server-side, or both)
🎉 This PR is included in version 11.6.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changes
This handling seems to have become incorrect over the past few months. This makes it correct.
Screenshots