bhauman / devcards

Devcards aims to provide a visual REPL experience for ClojureScript
1.53k stars 113 forks source link

Require react directly #165

Closed filipesilva closed 4 years ago

filipesilva commented 4 years ago

Hi there,

Devcards seems to depend on the global React instead of requiring it directly. When using https://github.com/thheller/shadow-cljs with Devcards, you need to directly import cljsjs.react and cljsjs.react.dom before importing devcards.core, otherwise it will error out at runtime:

  (:require [cljsjs.react]
            [cljsjs.react.dom]
            ; devcards needs cljsjs.react and cljsjs.react.dom to be imported
            ; separately for shadow-cljs to add shims.
            [devcards.core :refer [start-devcard-ui!]]
            ...

If devcards imports React directly instead, it should work though:

(:require ["react" :as react])
lfborjas commented 4 years ago

I ran into this same issue! If you're using React and ReactDOM from NPM (and maybe also from CLJSJS, though that's not my case,) you can also export those globals in your shadow configuration instead of explicitly requiring the cljsjs ones--my project uses NPM dependencies, and I didn't want to use CLJSJS only for the benefit of a dev tool hehe.

e.g. (the relevant bit being the :js-options entry, rest is for context: )

:devcards
    {:target :browser
     :output-dir "target/cljsbuild/public/js/devcards"
     :asset-path "/js/devcards"
     :compiler-options {:devcards true}
     :modules {:main {:entries [myapp.devcards]}}
     :js-options
     {:resolve
      {"react" {:export-globals ["React"]}
       "react-dom" {:export-globals ["ReactDOM"]}}}

However, I would also like to see this resolved in the library to not have to reach for this relatively arcane Shadow CLJS option (I only found out about it while perusing the shadow source code and finding this fun gem: https://github.com/thheller/shadow-cljs/blob/989590bb373e56b1d0ccf3a4029e2d3ec294ffe3/src/main/shadow/build/resolve.clj#L69)

Also, caveat, this codebase assumes that React is a globally defined property of js, there's a few js/Reacts scattered in a handful of files, even though the require you propose is already in place. There's an old PR by one of the maintainers of Reagent where this was pointed out and later fixed in a slightly different way. Finding out that these globals were being presupposed is what led me to configure Shadow to provide them, felt cleaner than requiring React in a different way than the strategy I use for my actual app.

filipesilva commented 4 years ago

Requiring cljsjs.react or cljsjs.react.dom with shadow-cljs will actually use the shims in https://github.com/thheller/shadow-cljsjs, and these are included by default (somewhat related to https://github.com/bhauman/devcards/issues/156).

So following my repro doesn't actually need cljsjs, but now that I think about it some more it is extra confusing to be referring to something that isn't anywhere in the deps too.

Looking at https://github.com/bhauman/devcards/pull/127 I guess this was meant be fixed in 0.2.6, but isn't? I suppose the js/React and js/ReactDOM references you mentioned, and are different between that PR and 4881652a48bece35f25b7d8aa1e39f73ecacb751, are to blame.

suud commented 4 years ago

It has been fixed in v0.2.7.

filipesilva commented 4 years ago

Right you are @suud!