Conversation
|
👋 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 |
| } | ||
| }, | ||
| "compilerOptions": { | ||
| "lib": ["ESNext"], |
There was a problem hiding this comment.
These can be moved to @octokit/tsconfig later on
| "@octokit/webhooks-methods": "^4.0.0", | ||
| "@wolfy1339/openapi-webhooks-types": "5.1.3", | ||
| "aggregate-error": "^3.1.0" | ||
| "aggregate-error": "^5.0.0" |
There was a problem hiding this comment.
While AggregateError was introduced natively in NodeJS v15, it isn't generic. So the types cannot be extended to use our custom error
| ? EmitterWebhookEvent | ||
| : EmitterWebhookEvent & TTransformed; | ||
| } | ||
|
|
There was a problem hiding this comment.
The types were fixed in version 3.0.1 and these are out of date versus v5 of aggregate-error
| expect(error.message).toMatch(/oops/); | ||
|
|
||
| const errors = Array.from(error); | ||
| const errors = Array.from(error.errors); |
There was a problem hiding this comment.
The AggregateError class doesn't extend Iterable<T> anymore, you have to use the .errors property
There is currently a bug in ts-node where it completely ignores the `module` and `moduleResolution` options kulshekhar/ts-jest#4198
nickfloyd
left a comment
There was a problem hiding this comment.
All of this looks good @wolfy1339 - thank you for pushing this forward ❤️. Just a clarifying question - are you planning a follow along set of changes in octokit.js and app.js for imports and such since those are still cjs modules?
|
Yes, I will be doing the other modules as well. |
|
🎉 This PR is included in version 13.0.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
BREAKING CHANGE: Migrate to ESM builds