-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat(typescript): deduplicate Sender
type
#397
Conversation
54adf90
to
21adc17
Compare
@wolfy1339 that was bad timing on my part 😅 - I just dropped the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think before we add more workarounds like this, we should create a proper schema via https://github.com/octokit/webhooks.js/pull/397/files. There are tools to translate a JSON Schema to TypeScript. We could probably reuse schemas from the official OpenAPI Spec from the activity endpoints: https://raw.githubusercontent.com/octokit/openapi/main/generated/api.github.com.json
🎉 This PR is included in version 7.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@gr2m I agree that's probably going to be the better way to go - I've had decent experience with both writing json schemas & generating types from them, so happy to help with improving the schemas once that PR is landed, and with generating good types from them. There's a few quick and easy de-duplications at the top level that I think would be good to land but otherwise agree that the time would be better spend focused on the schema route. That does however require the schemas to be landed - I'd be happy to help finish it off, but not the PR author so what I can do it limited 🙂 |
You can always send another PR for that. You could even re-use the commits from the original PR |
This was a bit of a test to see how easy it would be to do some de-duplication of common types, which looks promising.
The next blocker is needing to implement a way of getting all the property paths for an object since we can't use wildcards in
namedKeyPaths
which means we'd have to manually specify all the things to replace.This change is based on the fact that
sender
is always on every request.Interesting the
sender
forWebhookPayloadMarketplacePurchase
has anemail
property instead ofnode_id
- I've excluded that for now astype-writer
doesn't seem to support anyway to doextends
or&
:(