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

[MAINT]: drop aggregate-error dependency #1013

Closed
1 task done
isker opened this issue May 7, 2024 · 6 comments · Fixed by #1038
Closed
1 task done

[MAINT]: drop aggregate-error dependency #1013

isker opened this issue May 7, 2024 · 6 comments · Fixed by #1038
Labels
released Status: Needs info Full requirements are not yet known, so implementation should not be started Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@isker
Copy link

isker commented May 7, 2024

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

  • I agree to follow this project's Code of Conduct
@isker isker added Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels May 7, 2024
Copy link
Contributor

github-actions bot commented May 7, 2024

👋 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
Copy link
Member

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.

@wolfy1339 wolfy1339 added Status: Needs info Full requirements are not yet known, so implementation should not be started and removed Status: Triage This is being looked at and prioritized labels May 7, 2024
@isker
Copy link
Author

isker commented May 7, 2024

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
Copy link
Contributor

Uzlopak commented Jul 15, 2024

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.

Copy link
Contributor

🎉 This issue has been resolved in version 13.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@isker
Copy link
Author

isker commented Jul 15, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Status: Needs info Full requirements are not yet known, so implementation should not be started Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants