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

fix: implement some performance improvements #667

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

wolfy1339
Copy link
Member

  • Pre-compute the keys of the Endpoint type in order to not have to compute them multiple times
  • Use interface extends instead of intersections

See https://github.com/microsoft/TypeScript/wiki/Performance
Part of #666


Before the change?

  • Utilized keyof Endpoint directly in the code many times across different files
  • Used intersections for extending types

After the change?

  • Pre-compute the keys of the Endpoint type that can be shared across many files
  • Utilize interfaces and extends for better performance

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

* Pre-compute the keys of the `Endpoint` type in order to not have to compute them multiple times
* Use interface extends instead of intersections

See https://github.com/microsoft/TypeScript/wiki/Performance
Part of #666
@wolfy1339 wolfy1339 added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Mar 13, 2025
Copy link
Contributor

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@MisanthropicBit
Copy link

I'll try out this branch in my projects and see if it fixes in the issue in #666 🙂

@wolfy1339
Copy link
Member Author

It should definitely improve things, that's for sure.

When I tested some small test code for Octokit, the TypeScript profiler did identify the hotspot with the keyof Endpoint, other than that there wasn't anything else

@MisanthropicBit
Copy link

I tried testing with the branch locally (outside docker) but it seems the issue persists with the same tracing profile as before. I think this PR is still an overall improvement though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Status: 🆕 Triage
Development

Successfully merging this pull request may close these issues.

2 participants