reagent icon indicating copy to clipboard operation
reagent copied to clipboard

Impossible to use Reagent components after reactify-component + adapt-react-class

Open p-himik opened this issue 6 years ago • 14 comments

I'm trying to use https://github.com/clauderic/react-sortable-hoc It requires wrapping your component in another one, which is done for you by the SortableContainer and SortableElement functions in that library. Example of how it should work: https://gist.github.com/vbedegi/0329db5ff5772526a34b56d59e07d320

The issue with this example is that it's oversimplified. As soon as I try to pass some CLJS maps inside props (e.g. :style for a re-com component), everything goes awry. It definitely has nothing to do with react-sortable-hoc itself, since even a simple thing like

(def (-> my-componenet
         react/reactify-component
         react/adapt-react-class))

will make the component useless. All nested maps (and most likely other structures) will be converted to a JS object and never converted back, so if I pass {:a {:b 1}} as props, the component ends up receiving {:a #js {:b 1}}. Now, if I try to be clever and unwrap all properties used in CLJS, so instead of (defn my-component [{:keys [a]}] ...) I get (defn my-component [& {:keys [a]}] ...), it doesn't work either because all subsequent arguments end up in props under the key :children. I doesn't seem like it's possible to circumvent this behavior and use a chain of react/reactify-component and react/adapt-react-class at all at this point.

p-himik avatar Jul 26 '18 12:07 p-himik

I think the root cause is that reagent.impl.template/convert-prop-value is deep, but its counterpart reagent.impl.component/props-argv is not.

p-himik avatar Jul 26 '18 12:07 p-himik

I guess one solution would be to add an argument to reagent.core/adapt-react-class to specify whether you want to use cljs->js for props or something less deep.

p-himik avatar Jul 26 '18 12:07 p-himik

I just ran into the same issue. When using adapt-react-class my props are transformed in a lossy fashion. I understand this to be a convenience feature for JavaScript-only components. Why not let users decide on the transformation function (if any) and pass the props value as-is?

(defn reagent-component []
  [:div
    ;; How I think it should work:
    [(r/adapt-react-class clazz) (cljs->js {:foo "bar"})]])

markusschlegel avatar Jul 26 '18 13:07 markusschlegel

At the moment my workaround is to wrap my values in an atom and immediately unpack it in the receiving component. Atoms are left intact by Reagent.

markusschlegel avatar Jul 30 '18 13:07 markusschlegel

@markusschlegel Atoms are left intact by clj->js that's used by Reagent. Actually, you could use any wrapper that's not a CLJS map or collection.

My initial solution was to just rewrite a couple of Reagent files and place them into my project. The solution works, and I wanted to create a PR, but I realized that the problem is not that simple. In a general case, if you wrap some Reagent component in a React wrapper, you may want to pass some arguments to the wrapper, and these arguments must be passed through clj->js. And there're the Reagent component's arguments that must not be passed through clj->js.

So, I think there're two ways to split the arguments:

  1. Wrap Reagent component's arguments in something, unwrap them in the component - that's what you and I implemented, although you chose an atom and I chose a small Reagent rewrite.
  2. Wrap all arguments in an object that implements ILookup (but not IMap) and IEncodeJS and pass all React wrapper's argument names to it so it knows what and what not to encode.

At this point, I'm still not sure whether the second way is viable, but it doesn't require any changes in Reagent (well, except that maybe this arguments wrapper should be implemented there) and does sound like the cleanest solution.

p-himik avatar Jul 30 '18 13:07 p-himik

I'm trying to use the same HOC with more complex children. The atom workaround I also came up with, but if feels a bit hacky that's for sure.

minikomi avatar Aug 09 '18 08:08 minikomi

Hitting the same problem using react-dnd in a project, so this is a +1 for finding a solution.

Rather than using an atom my workaround was to wrap my data in a JS object, i.e. #js {:data ...}. Same difference really, but probably a it cheaper.

I agree with @markusschlegel to give more control to the caller. A simple shallow conversion of the props map to a JS object (and back) should be sufficient. If you need to pass a number, string, or boolean, JS and CLJS values are the same anyway, if you want to pass an object or an array, the #js reader macro is perfectly convenient, e.g.

[wrapped-component {:jsProp      "A string", 
                    :otherJsProp #js {:a #js [1,2,3]}
                    :cljs-prop   {:just-a "reagular map"}}]

The deep conversion seems like a convenience too far. Of course there is compatibility issue. adapt-react-class-2? 😃

tslocke avatar Oct 13 '18 16:10 tslocke

If adapt-react-class doesn't work like you want, you can just call create-element and other React function directly, these wont transform the properties:

(defn wrapped-react-component [_props & children]
  ;; props are ignored in this example
  (apply r/create-element react-component
    #js {:jsProp "A string"
         :otherJsProp #js {:a #js [1 2 3]}}
         :cljs-prop {:just-a "regular map"}
    ;; convert children from Reagent hiccup to React
    (map r/as-element children))

Deraen avatar Oct 14 '18 18:10 Deraen

@Deraen Not sure I understand. r/create-element just calls react/createElement. But in my case, react-component is created by using a function that already returns an element: https://github.com/clauderic/react-sortable-hoc/blob/master/src/SortableHandle/index.js

An example of its intended usage: https://github.com/clauderic/react-sortable-hoc/blob/master/examples/drag-handle.js#L10

With that being said, I still tried your code - sure enough, I got all sorts of cryptic errors about not being able to convert symbols to string and stuff like that.

p-himik avatar Oct 19 '18 05:10 p-himik

@p-himik sortableHandle function doesn't return an element but a component. Element has to be instantiated from component using createElement.

Maybe react-sortable-hoc is a lib I should use for React lib interop example.

Deraen avatar Oct 19 '18 08:10 Deraen

I would definitely be interested in such an example!

p-himik avatar Oct 19 '18 08:10 p-himik

@p-himik Here's the example you linked converted to Reagent: https://github.com/reagent-project/reagent/blob/master/examples/react-sortable-hoc/src/example/core.cljs

Deraen avatar Oct 19 '18 10:10 Deraen

It could make sense to add hiccup "shortcut", similar to :> which only does create-element without adapt-react-class or create Reagent component logic (latter is what happens when fn is the first element in vector, and which is why React components can't directly be used as first element in hiccup forms).

Deraen avatar Oct 19 '18 10:10 Deraen

Considering adding two more shortcuts to control component/element creation: #494

Deraen avatar Apr 26 '20 09:04 Deraen