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 client #172

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Update octokit client #172

merged 2 commits into from
Nov 6, 2023

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Nov 3, 2023

I have been having issues with rate limiting on my PAT, and thought that the source of the issue might be KSv2. The Octokit SDK is supposed to have throttling best-practices built-in, but in the version of octokit we were using, it was commented-out: octokit/octokit.js@v1.7.1...v3.1.1#diff-0b622e16b7fc8551bbe85cced944e28ed91560455e117bfd5a2ffb91fa2731b6L13

So this PR is just updating the octokit client to latest. I tested by:

  • running npm i && npm run build
  • then in chrome://extensions make sure developer mode is enabled in the top-left corner
  • then press Load Unpacked, and choose the dist folder from k2-extension
  • Then open K2 and refresh the page. It should all continue to work
image
  • Open an issue and make sure the K2 sidebar appears as normal:
image

@roryabraham roryabraham requested a review from a team November 3, 2023 17:09
@roryabraham roryabraham self-assigned this Nov 3, 2023
@melvin-bot melvin-bot bot requested review from dangrous and removed request for a team November 3, 2023 17:09
@roryabraham
Copy link
Contributor Author

Breaking changes 1.0 -> 2.0:

image

Not affecting us since K2 runs in a browser and not node

Breaking changes 2.0 -> 3.0:

image
  • Node change doesn't affect this repo
  • Previews don't either – that's just for the Node REPL
  • We don't use agent in octokit.request anywhere in this repo
  • This one I'm slightly less confident in, but looking through src/js/lib/api.js, I couldn't find anything that looked like custom request options

tgolen
tgolen previously approved these changes Nov 3, 2023
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for submitting this. I agree with you that I don't think we are affected by the breaking changes.

@AndrewGable I thought you would get a kick out of Rory finding that this setting was commented out in their source code for the version we were using.

@AndrewGable
Copy link
Contributor

🤣🤣🤣 Great find

AndrewGable
AndrewGable previously approved these changes Nov 3, 2023
@roryabraham roryabraham dismissed stale reviews from AndrewGable and tgolen via f143e30 November 3, 2023 22:25
@roryabraham
Copy link
Contributor Author

Realized I forgot to bump the package version and such, uploaded those changes now

@tgolen tgolen merged commit 204decd into main Nov 6, 2023
3 checks passed
@tgolen tgolen deleted the Rory-OctokitThrottling branch November 6, 2023 16:02
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.

3 participants