octokit / webhooks.js

GitHub webhook events toolset for Node.js
MIT License
307 stars 80 forks source link

[MAINT]: drop aggregate-error dependency #1013

Closed isker closed 3 days ago

isker commented 2 months ago

Describe the need

The project's tsconfig targets es2023 and the stated node support is (at least?) node>16, but it still takes a dependency on aggregate-error, which is made redundant by a standard global for ~4 years now, introduced in versions older than these.

This dependency caused me trouble in an arcane monorepo rollup build that is mostly self-inflicted and probably experienced by nobody else, so I won't elaborate on the details. Instead I'll just appeal to the general principle that unneeded dependencies are bad 🌞.

SDK Version

No response

API Version

No response

Relevant log output

No response

Code of Conduct

github-actions[bot] commented 2 months ago

👋 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 Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

wolfy1339 commented 2 months ago

I would love to get rid of aggregate error, but one issue is that the built-in type is not generic. We use the type generic in our code unfortunately.

If you have a solution that is portable to all platforms let me know.

isker commented 2 months ago

I see.

I only have the mostly unhelpful observation that it should be possible to either wrap or extend the standard AggregateError type to satisfy the project's typing requirements without taking a runtime dependency on the third party implementation of AggregateError.

I see now that the aggregate-error type is in the project's distributed types, but it is pretty simple and could be copied into the project.

Uzlopak commented 4 days ago

Actually there is also a difference between aggregate-error and native AggregateError, when generating the message. aggregate-error basically concats the error messages of all the errors. Native AggregateError expects the message to be the second Parameter of the constructor.

So while we could integrate the generic type of AggregateError into our package, the actual breaking change at runtime would be the message part.

And probably this could have some sideeffects in probot, when the error needs to be logged. Needs a check in pino on how native AggregateErrors are logged.

github-actions[bot] commented 3 days ago

:tada: This issue has been resolved in version 13.3.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

isker commented 3 days ago

Thanks!