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

Upgrade @octokit/rest from v16.43.1 to v18.12.0 - Fixes #1202 #1204

Merged
merged 5 commits into from Feb 1, 2022

Conversation

fbartho
Copy link
Member

@fbartho fbartho commented Feb 1, 2022

Fixes #1202

This is blocked by: orta/memfs-or-file-map-to-github-branch#5

This PR incidentally upgraded TypeScript, which is also a breaking change (because .d.ts files could emit differently), but PR doesn't take advantage of the new TS features because husky's lint-staged step is still configured to use TSLint, and tslint doesn't understand import…type and optional-chaining ?. / null-coalescing , so it was blowing up my commits.

I intended to use 'type' imports & optional-chaining / null coalescing features, so I upgraded TS, but unfortunately, those features can't be used in this repo until tslint is replaced with ESLint
@fbartho fbartho requested a review from orta February 1, 2022 02:47
@fbartho
Copy link
Member Author

fbartho commented Feb 1, 2022

Note: the test failure is expected until: orta/memfs-or-file-map-to-github-branch#5 is merged and deployed!

@@ -85,7 +85,7 @@ const apiForDSL = (dsl: DangerDSLJSONType): Octokit | BitBucketServerAPI | GitLa
return new GitLabAPI(gitlab.metadata, getGitLabAPICredentialsFromEnv(process.env))
}

const options: Octokit.Options & { debug: boolean } = {
const options: ConstructorParameters<typeof Octokit>[0] & { debug: boolean } = {
Copy link
Member

Choose a reason for hiding this comment

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

Nice work

@orta
Copy link
Member

orta commented Feb 1, 2022

dts emit seems fine, but I think this is probably a semver major because we expose the octokit API object and if we had to make changes in our API usage there's a very good chance others will too

@orta orta merged commit fff2201 into main Feb 1, 2022
@fbartho fbartho deleted the fb/bump-octokit-rest branch February 1, 2022 08:46
fbartho added a commit that referenced this pull request Feb 1, 2022
tslint is end-of-life, and warns when you install it. Additionally, the version we were on didn't support 'import type' expressions, as well as optional-chaining & null coalescing when I tried to use those in #1204

This PR does update a bunch of tslint-disable comments, but otherwise tries to minimally change runtime source in danger-js. If we want to tighten the eslint rules, I'd be super supportive, but I didn't want to cause a ton of thrash, so mostly the rules that are enabled are the ones that don't trigger on tons of existing code.
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.

[Maintenance][Github] @octokit/rest is on version 18, but DangerJS is on 16
2 participants