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

SceneVariableSet: Store the whole error object instead of the error message #972

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

grafakus
Copy link

Currently, when an error happens during the validateAndUpdate observable / RxJS process, we only store the error message.

After this PR, the whole error object will be stored, which can then be used to to inspect its cause, stack traces or any other custom property.

This issue relates to #371

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this with ControlsLabel ?

We should the error in the Variable label component, currently it expects a string

@grafakus
Copy link
Author

grafakus commented Nov 27, 2024

Have you tested this with ControlsLabel ?
We should the error in the Variable label component, currently it expects a string

No, I missed it, fixed in b1b43b2

@grafakus grafakus requested a review from torkelo November 27, 2024 11:33
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would not logging the error be enough, do we need to store it?

@grafakus
Copy link
Author

grafakus commented Dec 19, 2024

would not logging the error be enough, do we need to store it?

Hi @torkelo , sorry I've been busy on other fronts... Now, the error message is logged with the console. What if the app uses a custom logger that's interested in the stack trace or other properties that the error contains? We need to store the whole error object to allow the custom logger to inspect it and send anything interesting about the error wherever it wants (the console or Faro or...).

What's your concern?

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See that error is very poorly typed on SceneVariableState

If we are to make this breaking change we need to change the error type to Error

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 this pull request may close these issues.

2 participants