Fork me on GitHub
#fulcro
<
2020-03-30
>
fjolne11:03:14

hi, there! i’ve put 2 Fulcro apps in one page for the first time, and something weird started to happen to rendering, looks like a race-condition between the apps, as either one of the randomly skips rendering on state updates, maybe anyone encountered this? happens for both keyframe and ident renderers, :force-root? true doesn’t help

tony.kay15:03:22

I’ve mounted two mount points from the same app, but not two apps before. The only thing I can think of that isn’t ideal for multi-apps on a page is if they both have components with the exact same fully-qualified names. That would cause the two apps to register component classes in the same registry, which would cause problems. I never liked the limitation the registry causes, but there are several features that are very difficult without it. That said, if you’re using the same code base you should really be using multiple mount point*s, not multiple apps. You can always do code splitting to dynamically load the parts if your app is getting too big, and the component registry actually makes *that much easier.

fjolne19:03:29

multiple mount points seem suitable for my case, thanks, i’ll try that out! and yes, I’ve also thought of component-registry being shared, but those apps have disjoint sets of full-qualified component names and i checked the registry by hand and it looks fine also, remounting both apps after the first transaction somehow fixes the problem

fjolne06:03:07

@tony.kay i’ve tried to use multiple mount points, but if i understand correctly, i should use the same root for both mount points, as every app is bound to exactly one root, right? but then i don’t quite understand how i would distinguish between those mount points and render different things on them

tony.kay13:03:59

Sorry, I was mis-remembering. I had remounted the same app between two different mount points.

tony.kay13:03:24

what is your use-case, and would portals work, since they can mount on external mount nodes from a single app?

fjolne15:03:22

the use case: we have two panels, one is contained in a div managed by a 3rd party lib (this lib manages its dimensions, so it must be there), and the other is contained outside of that 3rd party lib div (also a hard requirement, as otherwise it’s intertwining with event listeners etc) thus, we need 2 mount points simultaneously, and it would probably be pretty hard to contain this lib inside one all-consuming fulcro root (it’s a game engine)

tony.kay15:03:40

Use portals

tony.kay15:03:20

Mount the main app, then use a react portal to put the component of interest on the other div.

tony.kay15:03:31

Use floating roots support if it needs to act like a root (data-wise)

fjolne15:03:56

oh cool, didn’t know about portals, looks like a way to go, thanks! hope it won’t mess up the event bubbling...

tony.kay18:03:35

Note that react uses synthetic events that are all placed on the doc body. If you want an easier time of it, you might try preact. I just pushed a snapshot (3.1.23-SNAPSHOT) that will allow that if you add the following your your shadow-cljs build:

:hooks    {:target     :browser
                       :output-dir "resources/public/js/hooks"
                       :asset-path "/js/hooks"
                       :modules    {:main {:entries [other-demos.hooks-demo]}}
                       :js-options
                           {:resolve {"react"     {:target  :npm
                                                   :require "preact/compat"}
                                      "react-dom" {:target  :npm
                                                   :require "preact/compat"}}}
                       :devtools   {:after-load other-demos.hooks-demo/start
                                    :preloads   [com.fulcrologic.fulcro.inspect.preload]}}

tony.kay18:03:58

then in package.json pull in preact and preact/compat instead of the react stuff

tony.kay18:03:41

The main incompatibility I’ve found is that if you map over children, you’ll have to wrap that with (clj->js) to turn it into a js array…preact doesn’t use sequence walking like react does

tony.kay18:03:19

the reason I mention this is two-fold: preact doesn’t use synthetic events, so it might be less painful there. Also preact seems faster to me.

fjolne20:03:49

@tony.kay yeah, i’ve already had a funny time dealing with react events, thanks! swapable renderer is very cool

tony.kay15:03:33

technically, with advanced optimization, I’d sort of expect the two app registries to end up with different minified names, so that conflict might not occur under release builds.

tony.kay15:03:56

The again, if it is the same source base, you should be ending up with the same classes in the registry, so that would only break things if you had two builds, and two js files, loading in the same page in dev mode.

wilkerlucio16:03:20

@tony.kay I just did a test using the hooks support, I noticed its missing the part of setting context, I had to wrap my render with comp/with-parent-context, otherwise fulcro keeps complaining, I did a quick look on the code but I guess you may already know the best place to put the render wrapper

tony.kay16:03:07

@wilkerlucio you sure? Another person is using them I thought, and I’ve heard no complaints…I would think that would have just made them broken…

wilkerlucio16:03:25

yup, latest fulcro, my code:

(fc/defsc ParserAssistant
  [this {:ui/keys  [query-editor index-explorer]
         ::ui/keys [active-tab-id]}]
  {:pre-merge  (fn [{:keys [current-normalized data-tree]}]
                 (merge {::id               (random-uuid)
                         ::cp/parser-id     :base
                         ::ui/active-tab-id ::query
                         :ui/query-editor   {}
                         :ui/index-explorer {}}
                   current-normalized data-tree))
   :ident      ::id
   :query      [::id
                ::ui/active-tab-id
                ::cp/parser-id
                {:ui/query-editor (fc/get-query query.editor/QueryEditor)}
                {:ui/index-explorer (fc/get-query index.explorer/IndexExplorer)}]
   :use-hooks? true}
  (useEffect
    (fn []
      (initialize-parser-assistent this)

      js/undefined)
    #js [])

  (fc/with-parent-context this
    (ui/tab-container {}
      (ui/tab-nav {:classes           [:.border-collapse-bottom]
                   ::ui/active-tab-id active-tab-id
                   ::ui/target        this}
        [{::ui/tab-id ::query} "Query"]
        [{::ui/tab-id ::index-explorer} "Index Explorer"])
      (case active-tab-id
        ::query
        (query.editor/query-editor query-editor)

        ::index-explorer
        (index.explorer/index-explorer index-explorer)

        (dom/div "Invalid page")))))

tony.kay16:03:25

oh, rendering is fine

tony.kay16:03:30

transactions broken?

wilkerlucio16:03:46

no, render is a problem here, maybe because I'm missing hooks and non-hook components?

tony.kay16:03:56

mixing should be ok

wilkerlucio16:03:03

all the children are regular old components

tony.kay16:03:26

when do you get an error?

wilkerlucio16:03:54

if I remove the with-parent-context I get:

tony.kay16:03:26

weird…you’re using that component’s this to fix it? I wonder if hooks is more likely to use async parallel react rendering?

wilkerlucio16:03:51

yup, for a quick look in the code, it may be missing setting the dynamic things, but I'm not sure

wilkerlucio16:03:11

also I don't think its async, async would not work anyway with dynamic vars (I guess)

tony.kay16:03:15

the normal wrap render of the macro does that already

wilkerlucio16:03:40

let me try priting some of the dynamics

wilkerlucio16:03:45

to see if they have stuff

wilkerlucio16:03:48

yeah, confirmed, no dynamic bindings

wilkerlucio16:03:11

(fc/defsc ParserAssistant
  [this {:ui/keys  [query-editor index-explorer]
         ::ui/keys [active-tab-id]}]
  {:pre-merge  (fn [{:keys [current-normalized data-tree]}]
                 (merge {::id               (random-uuid)
                         ::cp/parser-id     :base
                         ::ui/active-tab-id ::query
                         :ui/query-editor   {}
                         :ui/index-explorer {}}
                   current-normalized data-tree))
   :ident      ::id
   :query      [::id
                ::ui/active-tab-id
                ::cp/parser-id
                {:ui/query-editor (fc/get-query query.editor/QueryEditor)}
                {:ui/index-explorer (fc/get-query index.explorer/IndexExplorer)}]
   :use-hooks? true}
  (useEffect
    (fn []
      (initialize-parser-assistent this)

      js/undefined)
    #js [])

  (js/console.log "BEFORE" fc/*app* fc/*parent*)
  (fc/with-parent-context this
    (js/console.log "AFTER" fc/*app* fc/*parent*)
    (ui/tab-container {}
      (ui/tab-nav {:classes           [:.border-collapse-bottom]
                   ::ui/active-tab-id active-tab-id
                   ::ui/target        this}
        [{::ui/tab-id ::query} "Query"]
        [{::ui/tab-id ::index-explorer} "Index Explorer"])
      (case active-tab-id
        ::query
        (query.editor/query-editor query-editor)

        ::index-explorer
        (index.explorer/index-explorer index-explorer)

        (dom/div "Invalid page")))))

tony.kay16:03:00

Did you make a factory for it?

tony.kay16:03:41

I guess you must have

wilkerlucio16:03:30

Im using as a workspaces card

wilkerlucio16:03:43

so its wrapped and the wrapper is a regular fulcro component, and it makes a factory for the child as well

tony.kay16:03:01

really strange. the bindings are done at the very top…factories put the bindings onto this so with-parent-context will work

tony.kay16:03:06

not sure how they would get lost

tony.kay16:03:55

Perhaps it is time to move those out of bindings and into React context…

wilkerlucio16:03:20

@tony.kay I'm not sure the factory does what you are saying

wilkerlucio16:03:30

(defn factory
  "Create a factory constructor from a component class created with
   defsc."
  ([class] (factory class nil))
  ([class {:keys [keyfn qualifier] :as opts}]
   (let [qid (query-id class qualifier)]
     (with-meta
       (fn element-factory [props & children]
         (let [key              (:react-key props)
               key              (cond
                                  key key
                                  keyfn (keyfn props))
               ref              (:ref props)
               ref              (cond-> ref (keyword? ref) str)
               props-middleware (some-> *app* (ah/app-algorithm :props-middleware))
               ;; Our data-readers.clj makes #js == identity in CLJ
               props            #js {:fulcro$value   props
                                     :fulcro$queryid qid
                                     :fulcro$app     *app*
                                     :fulcro$shared  *shared*
                                     :fulcro$parent  *parent*
                                     :fulcro$depth   *depth*}
               props            (if props-middleware
                                  (props-middleware class props)
                                  props)]
           #?(:cljs
              (do
                (when key
                  (gobj/set props "key" key))
                (when ref
                  (gobj/set props "ref" ref))
                ;; dev time warnings/errors
                (when goog.DEBUG
                  (when (nil? *app*)
                    (log/error "A Fulcro component was rendered outside of a parent context. This probably means you are using a library that has you pass rendering code to it as a lambda. Use `with-parent-context` to fix this."))
                  (when (or (map? key) (vector? key))
                    (log/warn "React key for " (component-name class) " is not a simple scalar value. This could cause spurious component remounts."))

                  (when (string? ref)
                    (log/warn "String ref on " (component-name class) " should be a function."))

                  (when (or (nil? props) (not (gobj/containsKey props "fulcro$value")))
                    (log/error "Props middleware seems to have the corrupted props for " (component-name class))))))
           (create-element class props children)))
       {:class     class
        :queryid   qid
        :qualifier qualifier}))))

wilkerlucio16:03:42

for what I understood so far, the one doing it is the defsc, but wrapping the :render

wilkerlucio16:03:51

and I'm guessing its missing for the hook part

tony.kay16:03:10

components.cljc:1227

tony.kay16:03:13

is the hooks one

wilkerlucio16:03:31

yeah, I'm still trying to find where it calls the wrap-base-render

wilkerlucio16:03:37

that's the thing I'm guessing its missing

wilkerlucio16:03:39

the wrapper on 1232 seems to add extensions, but not the base things that wrap-base-render does

tony.kay16:03:13

1219 is the one for regular components

tony.kay16:03:31

The bindings happen once, at the top of the tree.

tony.kay16:03:51

The factories copy them into the react elements for the cases where you need them async

tony.kay16:03:57

(with-parent-context)

tony.kay16:03:14

oh wait…you’re right

tony.kay16:03:16

I was mis-remembering

wilkerlucio16:03:14

I see that happening on configure-component!, which is not called for the hooks version

wilkerlucio16:03:42

line 387 does it in the configure-component! case

tony.kay16:03:01

right, missed that, that is most likely the problem

tony.kay16:03:16

integrating with async crap in js is fun…

wilkerlucio16:03:36

but in this case there is nothing about async, does it?

tony.kay16:03:59

have to re-establish Fulcro dynamic bindings from various entries…and setState is async

tony.kay16:03:04

and not from root

tony.kay16:03:22

so, doing binding from root won’t ever work

tony.kay16:03:48

now that they’ve standardized Context (wasn’t in React 15) it is tempting to revisit that and see if it can be simplified,…it’s really ugly

tony.kay16:03:55

oh, no, context would actually not be great either, since we need dynamic bindings like parent, which change as you traverse the tree

tony.kay16:03:24

worth giving it a try I guess…might work out

tony.kay16:03:09

it would at least work for app, shared, and query state

wilkerlucio16:03:18

I didn't played that much with context, seems pretty similar, if the async parts can make it easier, could be interesting

tony.kay16:03:51

right, that’s the part that I’m thinking might be useful…since it’s a built-in feature of React, we don’t have as much to worry about in terms of them changing how rendering works

tony.kay16:03:37

*depth* is important to render optimizations on our end, though, and *parent* is needed by things like localized-dom.

tony.kay16:03:48

both of those change as you traverse the tree synchronously

tony.kay16:03:52

you making a patch for hooks @wilkerlucio?

wilkerlucio16:03:28

@tony.kay not yet, but I can give a go to it later

tony.kay17:03:46

@wilkerlucio try 3.1.23-SNAPSHOT

tony.kay17:03:57

I put in a quick patch

Jakub Holý (HolyJak)17:03:10

@tony.kay Is there a way to get the current report params inside an ::report/row-actions action? Other than instance -> app -> look it up manually in the DB? Hm, from BooleanInputit seems that params are in the props...

4
wilkerlucio17:03:15

@tony.kay just tried, with this version I can't load my app at all:

wilkerlucio17:03:15

seems like any->app is not working on this in this context

tony.kay17:03:12

ok, well, I don’t have any more time to play with it…I’ve pushed those changes…feel free two tweak

tony.kay17:03:23

it’s strange that I wrote this code when I did the original work, and it worked, including txes.https://github.com/fulcrologic/fulcro/blob/develop/src/todomvc/roots/hooks_demo.cljs

tony.kay17:03:38

did a to-one and a to-many

tony.kay17:03:45

all was working I thought

wilkerlucio17:03:26

I have a bug finger XD haha

wilkerlucio17:03:47

but no worries, I'll have some time to look on it a bit later today

wilkerlucio17:03:55

I'll send a PR with the progress

wilkerlucio17:03:42

@tony.kay maybe you didn't hit the bug because you are not using localized css?

wilkerlucio17:03:55

the component that was complaining here was a simple UI component (just css)

tony.kay17:03:34

I’m going to undo my patch

tony.kay17:03:39

and force push develop

tony.kay17:03:45

and put it on a branch

tony.kay17:03:48

branch is hooks-bug

wilkerlucio17:03:08

@tony.kay related to that, on my side I wrote a use-effect that's a bit different than yours, mine looks like this:

(defn wrap-effect [f]
  (fn []
    (let [res (f)]
      (if (fn? res)
        res
        js/undefined))))

(defn use-effect
  ([f]
   (useEffect (wrap-effect f)))
  ([f args]
   (useEffect (wrap-effect f) (to-array args))))

tony.kay17:03:23

I didn’t have chilren

tony.kay17:03:27

so I would not have seen it

tony.kay17:03:34

I’m adding that to the hooks demo code now

wilkerlucio17:03:53

given in cljs we are always returning things, and use-effect only accept fn or undefined

tony.kay17:03:55

and that is showing the breakage

🆒 4
wilkerlucio17:03:41

also, using to-array instead of clj->js is faster and less intrusive (keeps internal CLJS things, need to test if that has a good comparator)

tony.kay17:03:22

I think #js happens at compile time

wilkerlucio17:03:39

I'm talking about:

(defn use-effect
  "A simple wrapper around React/useEffect that auto-converts cljs arrays of deps to js."
  ([f] #?(:cljs (js/React.useEffect f)))
  ;; TODO: optimization: if this were a macro we could convert literal vectors at compile time. See DOM macros.
  ([f deps] #?(:cljs (js/React.useEffect f (clj->js deps)))))

tony.kay17:03:53

no no, I mean if you want to optimize it fully, you should pass a #js array

tony.kay17:03:58

then clj->js is a no-op

wilkerlucio17:03:19

yeah, I'm thinking about the convience usage from use-effect

wilkerlucio17:03:32

we may want to have the original clojure datastructures there (not converting to JS)

tony.kay17:03:47

yeah, if the top-level thing is js, it just does nothing

tony.kay17:03:58

anyway diff topic

tony.kay17:03:06

and probably of no consequence

wilkerlucio17:03:20

yeah, just thinking about the common usage pattern

wilkerlucio17:03:34

as a cljs fn, I like to use CLJS structures, using #js there feels weird

wilkerlucio17:03:50

(if it is to use #js there, might just go strait to the react version)

tony.kay17:03:00

AH…use-state

tony.kay17:03:36

updating via txn seems ok, but through use-state set I see weirdness

tony.kay17:03:40

so a lot of cases are working…the child, for example, can transact…it’s a call to a use-state’s update function that seems to cause issues

tony.kay17:03:01

(and the optimized renderers do that in order to update the component’s props side-band)

tony.kay18:03:11

@wilkerlucio try that snapshot again

wilkerlucio18:03:47

@tony.kay testing in a bit, but just to clarify, in my case I wasn't using useState at all

tony.kay18:03:05

ok. I was able to reproduce and fix at least one issue

tony.kay18:03:36

so, it is possible there other others. If you find one, a repro in the hooks-demo ns on that branch would be appreciated

wilkerlucio18:03:25

@tony.kay now I'm getting The required namespace "com.fulcrologic.fulcro.inspect.preload" is not available.

wilkerlucio18:03:18

nevermind, I think its a dep cache issue

wilkerlucio18:03:38

@tony.kay latest snapshot working here 👍

👍 4
tony.kay18:03:20

it wasn’t ideal patch…need to do a bit more tracing, but if it works for now, good enough