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

error callback / complete callback are not actually used in inspect #174

Closed
Jamesernator opened this issue Sep 15, 2024 · 4 comments · Fixed by #176
Closed

error callback / complete callback are not actually used in inspect #174

Jamesernator opened this issue Sep 15, 2024 · 4 comments · Fixed by #176

Comments

@Jamesernator
Copy link
Contributor

Jamesernator commented Sep 15, 2024

The inspect method defines error callback and complete callback, however it never actually calls them inside error/complete observer methods.

The steps should probably be similar to the steps for the next callback (i.e. passing thrown errors to subscriber.subscribe).

@Jamesernator
Copy link
Contributor Author

One consideration, perhaps for error we should be using SuppressedError from the (currently being implemented) explicit resource management proposal for the error case.

i.e. The steps would be equivalent to:

error: (error) => {
    try {
        errorCallback?.(error);
        subscriber.error(error);
    } catch (inspectError: unknown) {
        subscriber.error(
            new SuppressedError(
                inspectError,
                error,
                `inspector error callback threw an error`,
            ),
        );
    }
},

@domfarolino
Copy link
Collaborator

Thanks for catching this. I'll fix the immediate bug to bring it inline with the implementation (#176), and link to #150 to consider the explicit resource management proposal as a follow-up.

@domenic
Copy link
Collaborator

domenic commented Sep 22, 2024

I think it might be worth a separate issue to discuss the SupressedError idea in particular, as it's a change in semantics, apart from the addition of Symbol.dispose.

@domfarolino
Copy link
Collaborator

Done (#177) thanks!

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.

3 participants