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.

Sign up for GitHub

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

Jump to bottom

fix(types): improvements to internal types and options parameter #642

Merged
merged 13 commits into from
Nov 18, 2023
Merged

fix(types): improvements to internal types and options parameter #642

merged 13 commits into from
Nov 18, 2023

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented Nov 7, 2023 •
edited
Loading

Fixes #641

This should unblock users, as using object doesn't allow users to access options properties in TypeScript.

The types were inferred from the call to octokit.hook.wrap(), and also confirmed from the types from@octokit/core

https://github.com/octokit/core.js/blob/1c64ce28a2352c88091732985502b257cb242c02/src/types.ts#L55-L59

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented label Nov 7, 2023
gr2m
gr2m previously approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

cheers!

@gr2m
Copy link
Contributor

gr2m commented Nov 7, 2023

@octokit/maintainers you can remove the required CI from all @octokit repositories using this @octoherd script: https://github.com/octoherd/script-remove-required-ci-check

@wolfy1339
chore: remove most ts-expect-error annotations
973e3e9 
* Add internal `State` type
* Type the `wrapRequest()` function
* Add type augmentation of `OctokitResponse`
@wolfy1339
fix: improve options type
44d3def
@wolfy1339 wolfy1339 mentioned this pull request Nov 7, 2023
Merged
4 tasks
@wolfy1339 wolfy1339 added the Status: Blocked Some technical or requirement is blocking the issue label Nov 7, 2023
@wolfy1339
Copy link
Member Author

Waiting on octokit/types.ts#594

@wolfy1339 wolfy1339 requested a review from oscard0m November 7, 2023 02:59
@wolfy1339 wolfy1339 mentioned this pull request Nov 7, 2023
Closed
@wolfy1339
Copy link
Member Author

wolfy1339 commented Nov 7, 2023 •
edited
Loading

@oscard0m Could you review this to make sure it all makes sense

The types for the handler were taken from @octokit/core
https://github.com/octokit/core.js/blob/1c64ce28a2352c88091732985502b257cb242c02/src/types.ts#L55-L59

@oscard0m
Copy link
Member

oscard0m commented Nov 7, 2023 •
edited
Loading

@oscard0m Could you review this to make sure it all makes sense

The types for the handler were taken from @octokit/core https://github.com/octokit/core.js/blob/1c64ce28a2352c88091732985502b257cb242c02/src/types.ts#L55-L59

I won’t be available until next week to review this. Is that ok? @wolfy1339

@wolfy1339
Copy link
Member Author

That's fine

@wolfy1339 wolfy1339 removed the Status: Blocked Some technical or requirement is blocking the issue label Nov 9, 2023
oscard0m
oscard0m previously approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@oscard0m oscard0m left a comment

Choose a reason for hiding this comment

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

LGTM overall. Great job as always @wolfy1339

I left some comments on some @ts-expect-error, just in case there is a chance we can solve them.

src/index.ts Show resolved Hide resolved
test/exports.test.ts Show resolved Hide resolved
src/wrap-request.ts Outdated Show resolved Hide resolved
@wolfy1339 wolfy1339 requested a review from oscard0m November 15, 2023 01:44
@wolfy1339
Copy link
Member Author

This is odd... The typescript compiler doesn't complain when compiling the whole project for the types, it only complains for the validate:ts task

@wolfy1339
build: workaround typescript
a4ab07d 
Build the project first, then import from the build and then do the type check
@tarebyte
Copy link
Member

@octokit/maintainers you can remove the required CI from all

I'm on this team, but I'm not an org admin. Sorry!

@wolfy1339
Copy link
Member Author

This seems to do with the fact that typescript is compiling the files one by one, and not taking them into account as to how they affect other files.

When we build as a project, it keeps this context when it compiles all the files

oscard0m
oscard0m previously approved these changes Nov 18, 2023
View reviewed changes
@wolfy1339
Copy link
Member Author

Well this is annoying.... Merging in main dismissed the review

@wolfy1339 wolfy1339 enabled auto-merge (squash) November 18, 2023 15:44
@wolfy1339 wolfy1339 merged commit 2a64f7f into main Nov 18, 2023
7 checks passed
@wolfy1339 wolfy1339 deleted the improve-types branch November 18, 2023 15:45
@github-actions GitHub Actions
Copy link
Contributor

🎉 This PR is included in version 8.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscard0m oscard0m oscard0m approved these changes

@gr2m gr2m gr2m left review comments

Assignees
No one assigned
Labels
released Type: Bug Something isn't working as documented
Projects
🧰 Octokit Active
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LimitHandler typing uses type object for options
4 participants
@wolfy1339 @gr2m @oscard0m @tarebyte