-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[TwitchExtensionVersion] New badge #6900
[TwitchExtensionVersion] New badge #6900
Conversation
|
Honestly I don't really understand the |
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.
Hi @nixxquality . Thanks for working on this. Looks pretty good but I've left a couple of comments for things we can improve.
@PyvesB - sorry I wrote a few notes on this before noticing that you had assigned yourself the review. Since I've had a look at it I've posted the comments. Hopefully I'm not treading on your toes 🤞
I initially kept the schema that way because I figured that it would be fine since error responses have a radically different structure. I wrote a test to verify the Joi schema but it made the twitch-base code fail: t.create('possible schema change')
.skipWhen(noTwitchToken)
.get('/aaa5cu1nc9f4p75b791w8d3yo9d195.json')
.intercept(nock =>
nock('https://api.twitch.tv/helix/extensions/')
.get('/released?extension_id=aaa5cu1nc9f4p75b791w8d3yo9d195')
.reply(200, {data: [{title: "thing without version"}]})
)
.expectBadge({ label: 'twitch extension', message: 'not found' })
// resulted in
// Uncaught TypeError: Cannot read property 'statusCode' of undefined
// at TwitchExtensionVersion._requestJson (file:///C:/Users/Linus/Code/shields/services/twitch/twitch-base.js:94:60)
// for context, this is line 94:
if (err.name === 'InvalidResponse' && err.response.statusCode === 401) { Do Nock responses not come out the same way as regular responses? Anyway, I'm sure testing for negatives like this isn't a priority in a service's tests so I think we can ignore that. Since I'm using renderVersionBadge now I dropped the whole render function like other similar versioned badges do. |
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.
I think in this case, the HTTP interactions we would need to mock are more complicated than usual due to OAuth - _requestJson
() is doing a lot in TwitchBase
:
shields/services/twitch/twitch-base.js
Lines 73 to 114 in 17beba4
async _requestJson(request) { | |
// assign Client-ID and Authorization headers to request.options.headers if they are not set | |
request = { | |
...request, | |
options: { | |
...request.options, | |
headers: { | |
'Client-ID': this.authHelper._user, | |
Authorization: `Bearer ${await this._twitchToken()}`, | |
...(request.options && request.options.headers), | |
}, | |
}, | |
} | |
for (let i = 0; i < 3; i++) { | |
// 3 trials | |
try { | |
return await super._requestJson(request) | |
} catch (err) { | |
// if the token expire or is revoked | |
// https://dev.twitch.tv/docs/authentication/#refresh-in-response-to-server-rejection-for-bad-authentication | |
if (err.name === 'InvalidResponse' && err.response.statusCode === 401) { | |
TwitchBase.__twitchToken = this._getNewToken() | |
continue | |
} | |
// if API limit is exceeded | |
// https://dev.twitch.tv/docs/api/guide/#rate-limits | |
if (err.name === 'InvalidResponse' && err.response.statusCode === 429) { | |
const resetTimestamp = err.response.headers['ratelimit-reset'] | |
await sleep(Math.abs(resetTimestamp - Date.now()) + 100) | |
continue | |
} | |
// cannot recover | |
throw err | |
} | |
} | |
// one last time | |
return super._requestJson(request) | |
} |
You're right that we don't need an explicit test for validation fail at the service level though, so I think we can sidestep it.
This LGTM but I'll leave this open and see if @PyvesB wants to chime in on this before we merge it.
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.
Looks good to me as well, thanks @nixxquality ! 🎉
I've checked my Twitch API tokens setup, I don't believe any changes are required there to hit the new endpoint. Let's merge this!
Closes #6899
If you're wondering about
color: '6441A4'
it's taken from here:Twitch Purple #6441A4
.