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

feat(typescript): deduplicate Sender type #397

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

G-Rath
Copy link
Member

@G-Rath G-Rath commented Dec 15, 2020

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 for WebhookPayloadMarketplacePurchase has an email property instead of node_id - I've excluded that for now as type-writer doesn't seem to support anyway to do extends or & :(

@wolfy1339 wolfy1339 added the typescript Relevant to TypeScript users only label Dec 15, 2020
wolfy1339
wolfy1339 previously approved these changes Dec 15, 2020
@wolfy1339 wolfy1339 added the Type: Feature New feature or request label Dec 15, 2020
@G-Rath
Copy link
Member Author

G-Rath commented Dec 15, 2020

@wolfy1339 that was bad timing on my part 😅 - I just dropped the Webhook from the name to match PayloadRepository which is also deduplicated in the same way

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.

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

@gr2m gr2m merged commit 87b514e into octokit:master Dec 16, 2020
@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@G-Rath G-Rath deleted the de-duplicate-sender-type branch December 17, 2020 01:38
@G-Rath
Copy link
Member Author

G-Rath commented Dec 17, 2020

@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 🙂

@wolfy1339
Copy link
Member

You can always send another PR for that. You could even re-use the commits from the original PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request typescript Relevant to TypeScript users only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants