-
Notifications
You must be signed in to change notification settings - Fork 83
feat(typescript): deduplicate Sender type
#397
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
Conversation
54adf90 to
21adc17
Compare
|
@wolfy1339 that was bad timing on my part 😅 - I just dropped the |
gr2m
left a comment
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
namedKeyPathswhich means we'd have to manually specify all the things to replace.This change is based on the fact that
senderis always on every request.Interesting the
senderforWebhookPayloadMarketplacePurchasehas anemailproperty instead ofnode_id- I've excluded that for now astype-writerdoesn't seem to support anyway to doextendsor&:(