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

[BUG]: types for status codes are wrong #334

Open
1 task done
s-h-a-d-o-w opened this issue Jul 31, 2023 · 7 comments
Open
1 task done

[BUG]: types for status codes are wrong #334

s-h-a-d-o-w opened this issue Jul 31, 2023 · 7 comments
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented typescript

Comments

@s-h-a-d-o-w
Copy link

What happened?

The types for status codes usually only permit one status code, even though the API docs say differently.

Examples: pulls.list(...), repos.listBranches(...)

(Also: #188 - but that issue is more specific. This is obviously a widespread problem and a general fix would be great.)

Versions

Oktokit v19.0.13

Relevant log output

This condition will always return 'false' since the types '200' and '404' have no overlap.ts(2367)

Code of Conduct

  • I agree to follow this project's Code of Conduct
@s-h-a-d-o-w s-h-a-d-o-w added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Jul 31, 2023
@github-actions
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 labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@wolfy1339
Copy link
Member

Can you please tell us what version of typescript you are using, and what your tsconfig looks like?

I can probably have a look at this next week

@wolfy1339 wolfy1339 added typescript and removed Status: Triage This is being looked at and prioritized labels Jul 31, 2023
@wolfy1339 wolfy1339 self-assigned this Jul 31, 2023
@wolfy1339 wolfy1339 moved this from 🆕 Triage to 🏗 In progress in 🧰 Octokit Active Jul 31, 2023
@s-h-a-d-o-w
Copy link
Author

Typescript version: 5.1.6

tsconfig:

{
  "compilerOptions": {
    "target": "es2016",
    "module": "commonjs",
    "noEmit": true,
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true
  }
}

@wolfy1339
Copy link
Member

Closing as a duplicate of #188

@wolfy1339 wolfy1339 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 1, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in 🧰 Octokit Active Aug 1, 2023
@wolfy1339
Copy link
Member

Not exactly the same as #188

@wolfy1339 wolfy1339 reopened this Aug 1, 2023
@wolfy1339 wolfy1339 moved this from ✅ Done to 🔥 Backlog in 🧰 Octokit Active Aug 1, 2023
@wolfy1339
Copy link
Member

I have looked into this a little bit, and currently the types cannot handle an empty response for one status code and a body in another.

As for 404 status codes, they don't seem to be handled in the types at all

@s-h-a-d-o-w
Copy link
Author

s-h-a-d-o-w commented Aug 3, 2023

Glad you decided that it's not a duplicate. 🙂

As for 404 status codes, they don't seem to be handled in the types at all

Yes, after looking into this myself a bit, there's a couple of things:

  • I checked your docs for infos on said error behavior - there doesn't seem to be any.
  • I think types for errors would still be useful, since those can contain a whole bunch of data. At least by briefly glancing at them (I had to move on to other things), they looked structurally very similar to successful response bodies.

All of that said, in the case of pulls.list(...), it's not 404 but 304/422.
At least 304 admittedly might not be something that one will usually see, since I'm guessing it would require cache handling and presumably, there's no caching in octokit? I.e. when executing that function twice, the second time would not result in a 304?

And overall, it circles back to what I said in my OP - the two function calls I mentioned were just examples of what at least I perceive to be a general problem with non-2xx codes/responses when it comes to types (and possibly docs - which might be subjective).

@wolfy1339 wolfy1339 removed their assignment Nov 14, 2023
@wolfy1339 wolfy1339 added the Status: Up for grabs Issues that are ready to be worked on by anyone label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented typescript
Projects
Status: 🔥 Backlog
Development

No branches or pull requests

2 participants