Fork me on GitHub
#fulcro
<
2018-07-12
>
tony.kay02:07:56

hm… @danvingo That is definitely not an intended behavior 😜

tony.kay02:07:17

Not sure why that would be, but I’ll try reproducing it

tony.kay04:07:14

ok, that was premature 🙂 Working on it some more

tony.kay04:07:58

I don’t see an infinnite loop. There was a react key problem.

tony.kay04:07:50

new version on clojars. I’ve tried with react 15.6 and 16.4…both work for me.

myguidingstar10:07:20

for the sake of cache validation and garbage collection, what's the right way of marking entities in app db with timestamps when they were loaded?

wilkerlucio12:07:38

@tony.kay I just tested here, performance-wise it seems fine, but some stuff got broken, mostly related to things that I use modals/floaters (my guess is event related) with, I'm not sure why, it's not everywhere, but I have some modals that dont close once opened, other times I can open a popover once, then it stops working, and I'm getting no errors

wilkerlucio12:07:15

and I think there is still problems with react-key, seems like it's not working when I send :react-key "something" to the factory function call, eg (some-thing {:react-key "bla"})

wilkerlucio12:07:11

I didn't find the code for 2.6.0 in the fulcro repo, is that pushed?

tony.kay12:07:55

@wilkerlucio oops. I had not. feature/component-refinement. Some of the changes are probably going to be reversed. I was thinking I had to add separate hooks for upcoming v17, but this other approach seems better.

tony.kay12:07:02

it’s pushed now

wilkerlucio12:07:32

cool, what is the other approach?

tony.kay12:07:22

You’re right, I did break the react-key thing…let me fix that

tony.kay12:07:43

I was putting in hooks for the lifecycle rewrites so they could be different based on react version.

tony.kay12:07:08

but I was able to fix Fulcro to not need the deprecated lifecycle methods, so now it should just be fwd/bkwd compat

tony.kay12:07:17

all the way through async rendering

wilkerlucio12:07:30

I would like to try finding out why my events seems broken, but I'll probably only have time to do it late tomorrow

tony.kay12:07:11

try that new snapshot

tony.kay12:07:32

doubt that…could be render refresh issue…or the react key

tony.kay12:07:38

the react key should be fixed

wilkerlucio12:07:47

I have a lot minor event related things that seems to got broken after the update

wilkerlucio12:07:00

but like I said, not sure what/why, they just stopped working and I see no errors

tony.kay12:07:50

that really should not be…so I did decide on the data diffing we had discussed. I’m using the tree-path to the components to update the tree props and rendering from root (s.c.u short-circuits things that didn’t change)

tony.kay12:07:37

I guess it is possible the “path to the rendered data” (which ends up as an assoc-in update) is wrong in some corner cases? Are those always around unions?

tony.kay12:07:52

there could be a bug in the path generation algorithm

wilkerlucio12:07:18

I'm not using any unions, so must be something else

tony.kay12:07:54

all refreshes are proper “react way” (from root) now instead of tricky force renders…so it should have cleared up more bugs than it made 🙂

tony.kay13:07:23

I did change how react state is interfacing to the normal “react way”

tony.kay13:07:26

could be a bug there

tony.kay13:07:43

it is an observable difference

tony.kay13:07:20

set-state! is now a shallow merge, as it is in react

wilkerlucio13:07:24

the issues seem related to using events in the way I described here other day (using react elements to manage events, mostly keyboard or things outside the current dom tree)

tony.kay13:07:09

I didn’t touch anything related to events…so that’s just weird…React version change? Fulcro is using 16.4 now

wilkerlucio13:07:19

I'm still on 15.6

tony.kay13:07:39

(well, in that branch it is…I’ll default it back to 15 prob on release). Make sure it didn’t “upgrade” you…I guess you’re using shadow though?

tony.kay13:07:49

I did test with 15.6 too

wilkerlucio13:07:07

yeah, using shadow

tony.kay13:07:12

so that isn’t it

tony.kay13:07:45

ok, be happy to know what you find when you have time.

wilkerlucio13:07:05

can be related to lifecycle

wilkerlucio13:07:11

those event elements use then

tony.kay13:07:49

ok. Also, looks like I introduced a bug in refresh declared on mutations…I think the old path-meta function is stomping on metadata instead of updating it

tony.kay13:07:23

tak that back…false test failure

wilkerlucio13:07:17

@tony.kay I did a little experiment, and it seems like the render is failing to be called after some condition (not sure what yet)

wilkerlucio13:07:48

I'm triggering a value change on my component, but it's not calling the render method

wilkerlucio13:07:55

I see the value changing on inspect, but render is not called

tony.kay13:07:27

ok, that is what I would expect as a bug 🙂

tony.kay13:07:27

just a sec and I can push a version with some extra debug logic in it that would help

tony.kay13:07:23

pushed. Here’s the new logic: 1. refresh list comes in. 2. Find components in index (this is the “requested refresh”) 3. Find data path on each, and remove those whose parent will refresh (so no dupe renders) 4. Render the non-dupes

tony.kay13:07:44

There could be a bug in the dedupe logic, so I added log statements to show the “whole list” and the “pruned list”

wilkerlucio13:07:59

pushed on snapshot or just on the repo? (just asking because I tried updating deps and didn't got a new one)

tony.kay14:07:50

just pushed to git as well

wilkerlucio14:07:34

@tony.kay you forgot to use cljs reader macros on the debug info

wilkerlucio14:07:42

ExceptionInfo: failed to require macro-ns "fulcro.client.primitives", it was required by "fulcro.client.primitives"
	clojure.core/ex-info (core.clj:4739)
	clojure.core/ex-info (core.clj:4739)
	shadow.build.macros/load-macros/fn--16367/fn--16374 (macros.clj:73)
	shadow.build.macros/load-macros/fn--16367 (macros.clj:70)
	shadow.build.macros/load-macros (macros.clj:60)
	shadow.build.macros/load-macros (macros.clj:51)
	shadow.build.compiler/post-analyze-ns (compiler.clj:45)
	shadow.build.compiler/post-analyze-ns (compiler.clj:42)
Caused by:
CompilerException: java.lang.RuntimeException: Feature should be a keyword: (js/console.log :filtered-refresh (dedup-components-by-path components-to-refresh)), compiling:(fulcro/client/primitives.cljc:1949:91)
	clojure.lang.Compiler.load (Compiler.java:7521)
	clojure.lang.RT.loadResourceScript (RT.java:379)
	clojure.lang.RT.loadResourceScript (RT.java:370)
	clojure.lang.RT.load (RT.java:460)
	clojure.lang.RT.load (RT.java:426)
	clojure.core/load/fn--6548 (core.clj:6046)
	clojure.core/load (core.clj:6045)
	clojure.core/load (core.clj:6029)
Caused by:
RuntimeException: Feature should be a keyword: (js/console.log :filtered-refresh (dedup-components-by-path components-to-refresh))
	clojure.lang.Util.runtimeException (Util.java:221)
	clojure.lang.LispReader$ConditionalReader.hasFeature (LispReader.java:1509)
	clojure.lang.LispReader$ConditionalReader.readCondDelimited (LispReader.java:1545)
	clojure.lang.LispReader$ConditionalReader.invoke (LispReader.java:1645)
	clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:843)
	clojure.lang.LispReader.read (LispReader.java:275)
	clojure.lang.LispReader.readDelimitedList (LispReader.java:1384)

tony.kay14:07:29

sorry, cooking and coding

wilkerlucio14:07:41

no worries, I'm also doing other things, doing in bits when I can

tony.kay14:07:16

try that when you get a chance

tony.kay14:07:34

on clojars and git

wilkerlucio14:07:11

this component is a custom dropdown, when I just toggle to show and hide it works fine

wilkerlucio14:07:31

but when I pick an element, it goes away fine, but after that the component doens't trigger the render anymore

wilkerlucio14:07:49

(the boolean is the flag to if the dropdown is expanded or not)

tony.kay14:07:56

is it filtering out the render you expect?

tony.kay14:07:26

looks like a re-render of the container happens, but then only the dropdown

tony.kay14:07:56

so, what I want to know is: - Is the UI element that you expect to refresh even in the “requested”, and if so, does it errantly get removed? If it isn’t even making it into the refresh list that is one problem. If it is in the “filtered” list, but not rendering that is a shouldComponentUpdate bug probably. If it isn’t in the list, it could be a bug with lifecycle and indexing of the components

tony.kay14:07:15

and since I don’t know your code, I’m not sure what to expect in that list

wilkerlucio15:07:15

@tony.kay the log "RENDER" is from the Dropdown component, it is in both lists (refresh and filtered), but the actual render function is not called, if it were we would see the RENDER log

wilkerlucio15:07:41

from the options you mentioned, seems a bug in shouldComponentUpdate

tony.kay15:07:23

@wilkerlucio so I’m wondering what that component’s path is. When it does render, could you log out the metadata on props?

wilkerlucio15:07:26

sure, one sec

wilkerlucio15:07:14

after I do select an item, the meta turns into nil

wilkerlucio15:07:04

seems to happen when the parent is refreshed

tony.kay16:07:09

ok, so that is probably part of the problem…I’ll think through how that could happen. That path is important for the new refresh

tony.kay16:07:24

otherwise the props don’t get pushed into the right spot

wilkerlucio16:07:41

in this example it's a simple demo (not a big app), the DropdownContainer is the root (almost, there is one of those very clean aroudn it) and it has the Dropdown in

wilkerlucio16:07:49

when I just update the dropdown, it keeps working fine

wilkerlucio16:07:07

but when I trigger the action, this causes a mutation on the parent (DropdownContainer), the meta seems to get lost after that

wilkerlucio16:07:31

this one uses a lot of computed props, not sure if it makes a difference

tony.kay16:07:53

maybe computed is losing the metadata

wilkerlucio16:07:14

in this SS I was logging (meta props)

tony.kay17:07:40

is that one you can share with me…would help to have that repro case

tony.kay17:07:06

if I can get it to misbehave in a dev card it would be nice

wilkerlucio17:07:20

@tony.kay I just created a minimum repro for it

wilkerlucio17:07:29

(fp/defsc ComputedChild
  [this {:keys [my-value]} {:keys [on-select]}]
  {:initial-state (fn [_]
                    {::id (random-uuid)
                     :my-value "initial"})
   :ident         [::id ::id]
   :query         [::id :my-value]
   :css           []
   :css-include   []}
  (dom/div
    (dom/div "VALUE - " my-value)
    (dom/button {:onClick #(fm/set-value! this :my-value "A")} "Set local A")
    (dom/button {:onClick #(fm/set-value! this :my-value "B")} "Set local B")
    (dom/button {:onClick #(on-select "From inside")} "Call computed callback")))

(def input-component (fp/factory ComputedChild))

(fp/defsc DemoContainer
  [this {:keys [input ui/local-value]}]
  {:initial-state (fn [_]
                    {:input (fp/get-initial-state ComputedChild {})
                     :ui/local-value "initial"})
   :ident         (fn [] [:id "singleton"])
   :query         [:id {:input (fp/get-query ComputedChild)}
                   :ui/local-value]}
  (dom/div
    (dom/div "Outer - " local-value)
    (input-component (fp/computed input {:on-select #(fm/set-value! this :ui/local-value %)}))))

wilkerlucio17:07:43

when you click on the buttons of the ComputedChild, it updates fine

wilkerlucio17:07:59

but if you click to trigger the computed callback, after that the local thing on ComputedChild stops updating

wilkerlucio17:07:01

but it does update if you force a change on the DemoContainer (like by clicking on the "Call computed callback" button again)

wilkerlucio17:07:35

seems like updating the a parent is breaking the children capability to refresh itself

tony.kay23:07:24

@wilkerlucio should be fixed now

oscar23:07:14

@tony.kay This is by no means high priority, but I can't attach metadata to defsc because this line tramples it: https://github.com/fulcrologic/fulcro/blob/7ff0db1f42c49b7def5ed0409c70404b940e500f/src/main/fulcro/client/primitives.cljc#L3179

oscar23:07:31

If you want, I can open an issue and/or submit a PR.

cjmurphy23:07:27

Some debugging code left in I believe: :rnp :render :replacement-path.

tony.kay23:07:49

@oscar I’m in there…I’ll fix that

👍 4
tony.kay23:07:05

done…will be in 2.6

oscar23:07:17

Thank you!