Closed bchelkowski closed 6 months ago
@bchelkowski I heard that try/catch has a huge performance penalty and should be used for instance for third-party libs. I did not benchmark myself. Have you?
What is the motivation to have such handler? When something breaks and app crashes its better to let the box crash rather to supress. At least in dev env.
@tomek-r
I heard that try/catch has a huge performance penalty and should be used for instance for third-party libs. I did not benchmark myself. Have you?
I will try to benchmark it somehow. Could you link some resources that say so? I want to review them.
What is the motivation to have such handler? When something breaks and app crashes its better to let the box crash rather to supress. At least in dev env.
The motivation is to be able to address better app crashes.
This solution is not mandatory for Kopytko Components. Without writing the componentDidCatch
method, the app will just behave as it was before (it will crash).
But if someone prefers to handle such crashes in their way, for example by showing a screen with an error message, or reporting the reason for the crash to their destination reporting platform.
So it does not answer the question should whether the app should crash or not, just gives the possibility to choose what should happen.
I heard that try/catch has a huge performance penalty and should be used for instance for third-party libs. I did not benchmark myself. Have you?
I measured the most "fat" components in our application (using roTimespan
). Of course, I don't have a big test group so it is not statistically the best approach, but if the performance of try catch would be bad or tragic I would see some differences, but I don't see any, with this solution, and without it looks the same.
@bchelkowski
Could you link some resources that say so? I want to review them.
I think it was some discussion with Roku support. Have you reviewed https://developer.roku.com/en-gb/docs/references/brightscript/language/error-handling.md#proper-usage-of-exceptions?
I measured the most "fat" components in our application (using roTimespan). Of course, I don't have a big test group so it is not statistically the best approach, but if the performance of try catch would be bad or tragic I would see some differences, but I don't see any, with this solution, and without it looks the same.
It looks like it is optimized for no crash scenario.
componentDidCatch
function try/catch
is still executed. Can this be optional?@tomek-r
I think it was some discussion with Roku support. Have you reviewed https://developer.roku.com/en-gb/docs/references/brightscript/language/error-handling.md#proper-usage-of-exceptions?
Yes, I based on this documentation.
I am wondering if catching can be optional. In your proposal, if I don't override the componentDidCatch function try/catch is still executed. Can this be optional?
Yes, we can introduce bs_const in the app manifest (however the bs_const is mandatory so it will be a breaking change).
Does rethrowing change the source stack, for example shows the the file and line where you rethrow and not where the original crash happened?
Good point, a crash is pointing to the rethrown line, which won't be a transparent solution. So if we will introduce this feature it will be required to do it with bs_const.
@bchelkowski Can this be turned off by default?
I am wondering if we can have such handler per component meaning I can for instance have for a chosen component but not for all if bs_const feature flag is on.
@tomek-r
What do you think about this one? https://github.com/getndazn/kopytko-framework/pull/60/commits/3987423885d52b6fdddaa073225ce711667b6017
Without bs_const this implementation makes componentDidCatch transparent for current implementations.
@bchelkowski Wondering if we can inject #const to a file. But this would require each file modification via plugin?
I don't really follow. You mean some kind of new comment that will be converted to some kind of code value?
@bchelkowski
I don't really follow. You mean some kind of new comment that will be converted to some kind of code value?
So in Kopytko.brs file we can add a const with the value true/false and then depending on the value catch the function executions.
#const shouldUseTryCatch = false
sub initKopytko(dynamicProps = {} as Object, componentsMapping = {} as Object)
if (m._isInitialized) then return
m._kopytkoDOM.componentsMapping = componentsMapping
m._previousProps = _cloneObject(dynamicProps)
m.top.observeFieldScoped("focusedChild", "focusDidChange")
m.top.update(dynamicProps)
#if shouldUseTryCatch
try
constructor()
catch error
componendDidCatch(error)
end try
#else
constructor()
#end if
m._previousState = _cloneObject(m.state) ' required because of setting default state in constructor()
_mountComponent()
m._isInitialized = true
end sub
Something like that
# const shouldUseTryCatch = false
Would have to be appended during build time.
Also that means new flag would have to be needed for the config like useTryCatch: true/false
Ok, but why actually? If someone doesn't define componentDidCatch for the component it won't use try/catch.
Image the requirement. On prod env I want to report errors to some external service and degradate grafecully so I define componentDidCatch
. On dev env I want to have full, original stack in order to debug. Your proposition does not allow to do that. I don't want to comment out all componentDidCatch
functions. I want to switch it off for all on dev env without modifying the source code.
@tomek-r
Image the requirement. On prod env I want to report errors to some external service and degradate grafecully so I define
componentDidCatch
. On dev env I want to have full, original stack in order to debug. Your proposition does not allow to do that. I don't want to comment out allcomponentDidCatch
functions. I want to switch it off for all on dev env without modifying the source code.
:tada: This PR is included in version 3.0.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
BREAKING CHANGE: Add
enableKopytkoComponentDidCatch
bs_const to the manifest fileWhat did you implement:
The new Lifecycle method
componentDidCatch
is invoked when the error is thrown within the component.How did you implement it:
componentDidCatch(error as Object, info as Object)
How can we verify it:
Create a component, add
componentDidCatch
method, and make the component throw an error.componentDidCatch
implementationexample of throwing an error
componentDidMount
implementationTodos:
Is this ready for review?: YES Is it a breaking change?: YES