Skip to content

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

Closed
grafakus wants to merge 3 commits intomainfrom
grafakus/store-variable-error
Closed

SceneVariableSet: Store the whole error object instead of the error message#972
grafakus wants to merge 3 commits intomainfrom
grafakus/store-variable-error

Conversation

@grafakus
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor 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
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor 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
Copy Markdown
Collaborator

@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

@grafakus grafakus closed this Feb 19, 2026
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