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.

Sign up for GitHub

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

Jump to bottom

feat!: add componentDidCatch for error handling #60

Merged
merged 7 commits into from
Feb 16, 2024
Merged

feat!: add componentDidCatch for error handling #60

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
36ee519
feat: add componentDidCatch for error handling
bchelkowski Feb 12, 2024
3987423
feat: make componentDidCatch transparent for current implementations
bchelkowski Feb 13, 2024
2dd18d6
fix: method name
bchelkowski Feb 13, 2024
05e2c6d
chore: apply CR suggestions
bchelkowski Feb 13, 2024
8926608
feat: add more info fields
bchelkowski Feb 14, 2024
3b31ef8
feat!: add enableKopytkoComponentDidCatch bs_const
bchelkowski Feb 16, 2024
53f9bdb
chore: add _isComponentDidCatchEnabled
bchelkowski Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/renderer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,19 @@ these methods are called lifecycle methods:
end sub
```

- `componentDidCatch(error as Object, info as Object)` - called when a component method has thrown an error
```brightscript
sub componentDidCatch(error as Object, info as Object)
' The Roku exception object
' https://developer.roku.com/docs/references/brightscript/language/error-handling.md#the-exception-object
?error
' The info object containing
' componentMethod - component method where the error has been thrown
' componentName - node name that extends KopytkoGroup or KopytkoLayoutGroup
?info
end sub
```

Creating a tree of elements results in calling `constructor` method starting from the parent to children
and then `componentDidMount` in the opposite order - from children to the parent.

Expand Down
60 changes: 50 additions & 10 deletions src/components/renderer/Kopytko.brs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ sub init()
m.state = {}
m.elementToFocus = Invalid

m._enabledErrorHandling = Type(componentDidCatch) <> "<uninitialized>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m._enabledErrorHandling = Type(componentDidCatch) <> "<uninitialized>"
m._enabledErrorCatching = Type(componentDidCatch) <> "<uninitialized>"

What do you think?

Without bs_const I won't be able to catch original error (during development for example). Working with supressed or rethrown errors might be a nightmare

Copy link
Member Author

@bchelkowski bchelkowski Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think?

I prefer Handling, but it doesn't matter to me so much, I can change to Catching.

Without bs_const I won't be able to catch original error (during development for example). Working with supressed or rethrown errors might be a nightmare

But if someone is suppressing the error for some reason, it shouldn't crash.

Some app components could always extend KopytkoGroup and have any logic the dev wants, it could be embedded there, so for example it could throw an error if they want.
And use this component instead of the KopytkoGroup.

For example, JavaScript suppressed error won't be thrown.

Copy link
Member Author

@bchelkowski bchelkowski Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tomek-r

For example, with the current solution, if you want to handle an error but have it thrown for the dev environment, you can create your component that will extend KopytkoGroup and use the MyAppGroup component instead of KopytkoGroup.

<?xml version="1.0" encoding="utf-8" ?>

<component name="MyAppGroup" extends="KopytkoGroup">
  <script type="text/brightscript" uri="MyAppGroup.brs" />
</component>
sub componentDidCatch(error as Object, info as Object)
  #if isDevelopmentEnvironment
    ?info
    throw error
  #end if

  ' ... normal production code
end sub

Of course, it won't be the ideal solution as the backtrace of the crash won't be full,
but you still have the original error message and the original line that crashed.
The app is stopped also in the component scope (8085 port), so maybe you are not exactly at the line of the function that crashed, but you can check the component context and variables.

It is a trade-off solution I won't argue with that, but it is also an optional one, I don't want to force anybody to use it.

Maybe I will have some idea in the future how it could be improved but, don't have it now.

m._isInitialized = false
m._previousProps = {}
m._previousState = {}
Expand All @@ -21,7 +22,8 @@ sub initKopytko(dynamicProps = {} as Object, componentsMapping = {} as Object)
m.top.observeFieldScoped("focusedChild", "focusDidChange")
m.top.update(dynamicProps)

constructor()
_methodCall(constructor, "constructor")

m._previousState = _cloneObject(m.state) ' required because of setting default state in constructor()

_mountComponent()
Expand All @@ -32,7 +34,7 @@ end sub
sub destroyKopytko(data = {} as Object)
if (NOT m._isInitialized) then return

componentWillUnmount()
_methodCall(componentWillUnmount, "componentWillUnmount")

if (m["$$eventBus"] <> Invalid)
m["$$eventBus"].clear()
Expand All @@ -41,7 +43,8 @@ sub destroyKopytko(data = {} as Object)
m.state = {}
m._previousState = {}
m.top.unobserveFieldScoped("focusedChild")
m._kopytkoUpdater.destroy()

_methodCall(m._kopytkoUpdater.destroy, "destroyKopytko", [], m._kopytkoUpdater)

_clearDOM()

Expand Down Expand Up @@ -73,15 +76,15 @@ sub focusDidChange(event as Object)
end sub

sub setState(partialState as Object, callback = Invalid as Dynamic)
m._kopytkoUpdater.enqueueStateUpdate(partialState, callback)
_methodCall(m._kopytkoUpdater.enqueueStateUpdate, "setState", [partialState, callback], m._kopytkoUpdater)
end sub

sub forceUpdate()
m._kopytkoUpdater.forceStateUpdate()
_methodCall(m._kopytkoUpdater.forceStateUpdate, "forceUpdate", [], m._kopytkoUpdater)
end sub

sub enqueueUpdate()
m._kopytkoUpdater.enqueueStateUpdate()
_methodCall(m._kopytkoUpdater.enqueueStateUpdate, "enqueueUpdate", [], m._kopytkoUpdater)
end sub

sub updateProps(props = {} as Object)
Expand All @@ -92,10 +95,10 @@ end sub

sub _mountComponent()
m._virtualDOM = render()
m._kopytkoDOM.renderElement(m._virtualDOM, m.top)

m._kopytkoUpdater.setComponentMounted(m.state)
componentDidMount()
_methodCall(m._kopytkoDOM.renderElement, "renderElement", [m._virtualDOM, m.top], m._kopytkoDOM)
_methodCall(m._kopytkoUpdater.setComponentMounted, "setComponentMounted", [m.state], m._kopytkoUpdater)
_methodCall(componentDidMount, "componentDidMount")
end sub

sub _onStateUpdated()
Expand All @@ -114,7 +117,8 @@ sub _updateDOM()
m.top.setFocus(true)
end if

componentDidUpdate(m._previousProps, m._previousState)
_methodCall(componentDidUpdate, "componentDidUpdate", [m._previousProps, m._previousState])

m._previousState = _cloneObject(m.state)
end sub

Expand All @@ -131,3 +135,39 @@ function _cloneObject(obj as Object) as Object

return newObj
end function

sub _methodCall(func as Function, methodName as String, args = [] as Object, context = Invalid as Object)
if (m._enabledErrorHandling)
try
_functionCall(func, args, context)
catch error
_throw(error, methodName)
end try
else
_functionCall(func, args, context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Less nesting if return is appended and no need for else.

Copy link
Member Author

Choose a reason for hiding this comment

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

No need for a return if there is else. 😃

I can change to return. It doesn't matter to me.

end if
end sub

sub _functionCall(func as Function, args = [] as Object, context = Invalid as Object)
if (context = Invalid) then context = GetGlobalAA()

argumentsNumber = args.count()
context["$$functionCall"] = func

if (argumentsNumber = 0)
context["$$functionCall"]()
else if (argumentsNumber = 1)
context["$$functionCall"](args[0])
else if (argumentsNumber = 2)
context["$$functionCall"](args[0], args[1])
end if

context.delete("$$functionCall")
end sub
Copy link
Contributor

Choose a reason for hiding this comment

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

kopytko utils have function call function

Copy link
Member Author

Choose a reason for hiding this comment

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

True, will add utils function


sub _throw(error as Object, failingComponentMethod as String)
componentDidCatch(error, {
componentMethod: failingComponentMethod,
componentName: m.top.subtype(),
})
end sub
Loading