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

Add error boundary support, based on UIx v1 implementation #98

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

chrisetheridge
Copy link

  • Error boundary support for both cljs and JVM
  • Tests for both platforms
  • Documentation for error boundary recipe

I'm pretty much done with this one @roman01la . The only thing I'm not too happy about is having to access the children prop on the cljs side - (.-children children). Not entirely sure how to work around this. I guess we could do it automatically during the render-fn call?

(def error-boundary
  (uix.core/create-error-boundary
   {:derive-error-state (fn [error]
                          {:error error})
    :did-catch          (fn [error info]
                          (reset! *error-state error))}
   (fn [state [children]]
     (if (:error @state)
       ($ :p "Error")
       (.-children children)))))

(defn react-component-ctor []
(fn [^js/React.Component this _]
(set! (.-state this) #js {:argv nil})
(specify! (.-state this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say for UIx2 we should stick with hook-like API, use a tuple of [value set-value] instead of Atom-like wrapper, to be consistent with use-state hook. For UIx1 it made sense since local state there is wrapped as an atom.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense! I'll action

(this-as this
(let [args (.-argv (.-props this))
state (.-state this)]
(render-fn state args))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of state the render-fn would take [state set-state]

Copy link
Author

Choose a reason for hiding this comment

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

Cool, happy with that

{:error error})
:did-catch (fn [error info]
(reset! *error-state error))}
(fn [state [children]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

since in UIx2 components are single-arity functions taking a map of props, this part has to be updated as well. Instead of a vector of args the error boundary should pass in a map of props.

Copy link
Author

@chrisetheridge chrisetheridge Feb 27, 2023

Choose a reason for hiding this comment

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

Okay cool, so then the render-fn args would be identical to that of uix.core/fn right?

Edit - I see your new comment now, makes total sense

Copy link
Collaborator

Choose a reason for hiding this comment

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

@chrisetheridge see the example below #98 (comment)

@roman01la
Copy link
Collaborator

roman01la commented Feb 27, 2023

Given the above comments the final API should look like this:

(def error-boundary
  (uix.core/create-error-boundary
   {:derive-error-state (fn [error] {:error error})}
   (fn [[state set-state] {:keys [children]}]
     (if (:error state)
       ($ :p "Error")
       children))))

:componentDidCatch did-catch
:render render})]
(fn [& args]
(react/createElement class #js {:argv args}))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you add (set! (.-uix-component? class) true) and just return the class itself here instead of a function, then $ will pick it up and pass props properly, as a single map

- Error boundary support for both cljs and JVM
- Tests for both platforms
- Documentation for error boundary recipe
@chrisetheridge
Copy link
Author

Okay I think I've got it. Tests passing and works locally for me. I did have to use glue-args in cljs, when passing the props to the render-fn. I also put the component args into {:children args} in the JVM, to match the API in cljs

@roman01la
Copy link
Collaborator

Looking great!

@roman01la roman01la merged commit 28cc3dd into pitch-io:master Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants