search

omcljs / om

ClojureScript interface to Facebook's React
6.66k stars 363 forks source link

Om uses react but does not require it. #835

Open den1k opened 7 years ago

den1k commented 7 years ago

js/React is used on multiple occasions in om.next but never required as cljsjs.react

anmonteiro commented 7 years ago

this was done on purpose.

You should require om.dom in order to include React.

den1k commented 7 years ago

@anmonteiro why was this done on purpose? If om.next clearly uses js/React in cljs then why shouldn't it be required?

We have multiple .cljc files that contain query-only components. These are required from our main namespaces and components in both cljs and clj. Because they get loaded first we would have to require cljsjs.react in all .cljc files, where it is not actually used.

Peeja commented 7 years ago

Not everyone uses cljsjs to include React in their project, so Om opts not to force them to. It's up to you to bring your own React.

That said, I'm not sure what @anmonteiro means by "You should require om.dom in order to include React." om.dom doesn't require React either, presumably for the same reason.

anmonteiro commented 7 years ago

om.dom does require React from CLJSJS, see https://github.com/omcljs/om/blob/master/src/main/om/dom.cljs#L4

Peeja commented 7 years ago

Ah, my mistake, I missed dom.cljs and assumed dom.cljc was all there was.

Then, why does om.dom require cljsjs.react while the other bits of Om don't?

den1k commented 7 years ago

In that case I think a component should not use react if it only has static methods defined, i.e. when the component is only used for queries and normalization via idents. On Sun, Jan 8, 2017 at 7:47 PM Peter Jaros notifications@github.com wrote:

Ah, my mistake, I missed dom.cljs and assumed dom.cljc was all there was.

Then, why does om.dom require cljsjs.react while the other bits of Om don't?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/omcljs/om/issues/835#issuecomment-271170259, or mute the thread https://github.com/notifications/unsubscribe-auth/AEb7Is30BPQR4oa2xtzCQzwR8ZE3VtcZks5rQS9YgaJpZM4LaAWq .

swannodette commented 7 years ago

Why is this necessary? As in, what problem is this actually causing?

den1k commented 7 years ago

@swannodette as I said above

We have multiple .cljc files that contain query-only components. These are required from our main namespaces and components in both cljs and clj. Because they get loaded first we would have to require cljsjs.react or om.dom in all .cljc files, whose components don't even have a render function.