feat: migrate to ESM #968
feat: migrate to ESM #968
Conversation
wolfy1339
added
Type: Maintenance
|
👋 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 |
| @@ -8,6 +8,9 @@ | |||
| } | |||
| }, | |||
| "compilerOptions": { | |||
| "lib": ["ESNext"], | |||
There was a problem hiding this comment.
These can be moved to @octokit/tsconfig later on
| @@ -43,14 +44,13 @@ | |||
| "@octokit/request-error": "^5.0.0", | |||
| "@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
| @@ -75,20 +76,3 @@ export interface WebhookEventHandlerError<TTransformed = unknown> | |||
| ? 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
| @@ -91,12 +91,12 @@ describe("when a handler throws an error", () => { | |||
| } catch (error: any) { | |||
| 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
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