DeploysRadiator: fix when github return commits info without author login #13076
Conversation
| message: gitHubCommitJson.commit.message | ||
| } | ||
| )))); | ||
| } else { | ||
| return response.clone().json<GitHubErrorJson>() | ||
| // We need to re-assert the type for rejected promises | ||
| // https://github.com/Microsoft/TypeScript/issues/7588 | ||
| .then(json => Promise.reject<List<GitHubCommit>>(new Error('foo'))); | ||
| .then(json => Promise.reject<List<GitHubCommit>>(new Error(json.message))); |
There was a problem hiding this comment.
Took advantage of this commit to return real error message 👊
|
lgtm 👍 |
| @@ -43,15 +43,15 @@ export const getDifference = (base: string, head: string): Promise<List<GitHubCo | |||
| { | |||
| url: gitHubCommitJson.html_url, | |||
| authorName: gitHubCommitJson.commit.author.name, | |||
| authorLogin: gitHubCommitJson.author.login, | |||
| authorLogin: (gitHubCommitJson.author) ? gitHubCommitJson.author.login : 'Unknown', | |||
There was a problem hiding this comment.
This error slipped through the static type system because, currently in TypeScript, each type can be itself or undefined, e.g. Author = Author | undefined. This means you sometimes see foo is not a {property,method} of undefined errors slipping through the net. TypeScript doesn't know whether gitHubCommitJson.author is Author or undefined.
TypeScript 2 has a feature called control-flow based analysis which fixes this. It will follow the control flow of your program to determine the type. The above (gitHubCommitJson.author.login) would throw a compile error, but gitHubCommitJson.author && gitHubCommitJson.author.login would not. It forces you to guard these type of errors.
Another cool feature of TypeScript 2 is non-nullable types, which means Author = Author, instead of Author = Author | undefined.
There was a problem hiding this comment.
Thanks for the explanation. Looks like typescript 2 brings a lot of improvements. :)
What does this change?
If author of a commit is not a known github account (happens if your gitconfig email doesn't match your github email for instance), github api return
nullfor commit login. DeploysRadiator was not handling this case very wellWhat is the value of this and can you measure success?
https://frontend.gutools.co.uk/deploys-radiator doesn't return a blank page in the case described above
Does this affect other platforms - Amp, Apps, etc?
No
Request for comment
@NataliaLKB @lps88