futil-js / contexture-react

React components for building contexture interfaces
https://smartprocure.github.io/contexture-react
MIT License
6 stars 4 forks source link

Remove HOCs #582

Closed stellarhoof closed 1 year ago

stellarhoof commented 1 year ago
decrapifier commented 1 year ago
Warnings
:warning: :exclamation: This PR is BIG (+1765 -1641) Please keep it below 500 net changes

Generated by :no_entry_sign: dangerJS against 63e1071235dc8fb58d13f56278ab3a009e47d282

daedalus28 commented 1 year ago

One more thing on this PR - I'm not sure removing the HOC and inlining loaders everywhere is net gain, especially since we also want to add generic hover to trigger a focus/hover client state. We probably still want a way to provide off the shelf contexture wrapping for those things - a HOC actually seems like a good solution there. Open to alternatives, but duplicating the logic everywhere seems worse to me.

stellarhoof commented 1 year ago

With respect to loaders, I actually prefer the duplication because:

  1. We can customize where to put the loader. Perhaps we wanna be a bit fancier and only wrap an input instead of the entire component
  2. Less re-renders: a hoc will re-render the wrapped component if it re-renders itself, whereas a wrapper won't. I have not tested this, but I think it'll get rid of our problem with double rendering when loaders switch state.
  3. In general, hocs are harder to reason about (at least for me)

I'm not sure how the focus/hover feature would work so I can't comment there.

We can still export a hoc though, many libraries use this approach where they give you a few options on how to use their API (hook, hoc, render props, etc...)

stellarhoof commented 1 year ago

Will open again in the monorepo