-
Notifications
You must be signed in to change notification settings - Fork 27
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
No, I missed it, fixed in b1b43b2 |
There was a problem hiding this 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?
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? |
There was a problem hiding this 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
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