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

Should DisposeResources set cause or suppressed on AggregateError #112

Closed
rbuckton opened this issue Oct 24, 2022 · 3 comments · Fixed by #117
Closed

Should DisposeResources set cause or suppressed on AggregateError #112

rbuckton opened this issue Oct 24, 2022 · 3 comments · Fixed by #117
Milestone

Comments

@rbuckton
Copy link
Collaborator

Related to #104, specifically this question:

...

try {
  using c = { [Symbol.dispose]() { throw new Error("c"); };
  using b = { [Symbol.dispose]() { throw new Error("b"); };
} catch (e) {
  e
}

...

I also wonder if cause is the right term here, since Error("b") did not necessarily cause Error("c"), but rather it suppresses it. Perhaps I should install a suppressed property instead?

The DisposeResources operation will construct a jagged AggregateError with the above exceptions. Since b did not "cause" c, is this the right term we should be using? Should we consider defining a suppressed property instead of a cause property to more accurately reflect the relationship? Or should we continue to use cause for simplicity's sake?

/cc: @ljharb, @bakkot

@rbuckton rbuckton added this to the Stage 3 milestone Oct 24, 2022
@ljharb
Copy link
Member

ljharb commented Oct 24, 2022

I'm having trouble recalling why we wouldn't want it to always be an AggregateError, with no cause, containing all the errors that threw?

@rbuckton
Copy link
Collaborator Author

I'm having trouble recalling why we wouldn't want it to always be an AggregateError, with no cause, containing all the errors that threw?

This behavior was discussed in the last plenary, and was favored by @waldemarhorwat, @bakkot, and @syg. See #74 (comment) and https://github.com/tc39/notes/blob/main/meetings/2022-09/sep-15.md#explicit-resource-management-for-stage-3 for context. I'd like to keep this issue focused on the topic of naming.

Regardless as to whether the AggregateError is jagged or flat, this specific issue is related to the name of the property we install for the exception it suppresses. Currently, this is the cause property, however this seems like a misnomer given the reasoning I mentioned above.

@ljharb
Copy link
Member

ljharb commented Oct 24, 2022

I agree it's a misnomer, which unavoidably relates to the behavior, so I'm not sure how to decouple the two.

Adding a new property is a lot more trouble than it's worth imo; cause is on every error type, and adding a random own property seems problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants