Skip to content
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

Update @octokit/rest from v16 to v18 #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kwelch
Copy link
Collaborator

@kwelch kwelch commented Feb 22, 2021

I ran into some issues compiling an octokit library, so I update typescript and it worked.

@kwelch kwelch requested a review from jamiebuilds February 22, 2021 17:53
@kwelch kwelch force-pushed the kwelch-upgrade-octokit branch from e49ce52 to 5419121 Compare February 22, 2021 17:54
@kwelch kwelch changed the base branch from kwelch-fix-status-pagination to master February 22, 2021 17:55
@kwelch
Copy link
Collaborator Author

kwelch commented Feb 22, 2021

Updated to rebase from master. So it is independent of that other PR.

src/lib.ts Outdated
Comment on lines 64 to 72
let pullRequestCreator = pullRequest.data.user
? pullRequest.data.user.login
: "unknownCreator"
let pullRequestCommits = pullRequest.data.commits
let pullRequestBaseRef = pullRequest.data.base.ref
let pullRequestHeadRef = pullRequest.data.head.ref
let pullRequestUrl = pullRequest.data.html_url
let pullRequeseGitMergeable = pullRequest.data.mergeable
let pullRequeseGitRebaseable = pullRequest.data.rebaseable
let pullRequeseGitMergeable = pullRequest.data.mergeable || false
let pullRequeseGitRebaseable = pullRequest.data.rebaseable || false
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are now nullable, so I put in some defaults that are probably not amazing.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?? ?

src/lib.ts Outdated
string,
Octokit.PullsListReviewsResponseItem
> = new Map()
let latestReviews: Map<string, typeof reviews.data[0]> = new Map()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to the definitions of these maps are probably the most odd feeling part of this change. I am actually surprised it worked.

@kwelch kwelch self-assigned this Feb 22, 2021
src/lib.ts Outdated

for (let review of reviews.data) {
let prev = latestReviews.get(review.user.login)
const userLogin = review.user ? review.user.login : "unknownReviewer"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review.user?.login ?? "unknownReviwer"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 I was being lazy because I didn't want to update the babel configs.

@@ -119,10 +120,7 @@ export default async function isMergable(opts: Options): Promise<Result> {
pendingReviewRequests.push({ name: reviewRequest.login })
}

let latestStatuses: Map<
string,
Octokit.ReposListStatusesForRefResponseItem
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this type just not exist anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They moved to a plugin model and the type is no longer exported it. Actually I found @octokit/types, this looks like the correct option. Let me update to use that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I updated to use this type library, however there is some odd typescript thing happening. It is erroring on properties that typescript has the object is declared as having. Unfortunately, I don't know typescript well enough to debug it further. I attempted to add circle so it would be easier to show the error but I didn't have enough permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants