Fork me on GitHub
#fulcro
<
2020-05-09
>
holyjak07:05:34

I have extended a RAD report to have a "more info" popup as a row action. How did it go? Pretty well. The data is fetched on demand and I used: - ro/row-actions action with a custom :type :show-more and custom :content-class (open maps FTW!) - ro/row-query-inclusion was indispensable to add the query for the popup data (with :ui/... not to be loaded with the report) and its load marker - thanks to ::report/row-style I could copy the TableRowLayout and modify it to call my own version of row-action-buttons with extra code to handle :show-more, i.e. wrap the button with semantic-ui's Popup, leveraging the extra data from the action map What would have made my job easier is the ability to specify a custom control for a particular action :type, similarly as we can do for report controls with :com.fulcrologic.rad.control/type->style->control. Should I give it a shot and make a PR for this, @tony.kay?

currentoor17:05:35

hey that looks cool!

currentoor17:05:18

@tony.kay calls the shots on this sort of thing, but I definitely like what I’m seeing

tony.kay18:05:46

@holyjak So, the short answer is “yes”, but the longer answer is that I have not done this yet because there is a wider design concern that I’d like to consider: row-level controls, as you noticed, will often need to do something to the row-level query (unless they use a floating root, which is probably not generally advisable for something with lots of rows). It’s not going to hurt anything to add a report row action control style to semantic UI plugin, since it will be localized to that code. The generalization of “report controls” beyond simple actions has many other things we can do with it. We could, for example, work on allowing forms to be rows, so that the row is editable. That’s one extreme. Technically, the save-form function that is already available would make editable fields possible without any kind of “form” support…all you need is an id, attribute name, and value to build what save-form needs. So, action buttons at the moment is meant to be a simple abstraction around “here’s something I can do that doesn’t need anything more than a button”. The idea of making more powerful row-based controls is on the TODO list. The idea that row action buttons are a specialization of a more general control is really what I’d like to see.

tony.kay18:05:14

There’s an even wider thing to consider here as well: The fact that more powerful reports really need more than just row-based representations…you’re going to want things like “fold open details”, aggregation, etc.

tony.kay18:05:14

so, I’d say perhaps hold off on a PR for the moment and gain more experience by building things. You see how the hooks I’ve left leave it open for your expansion. I suspect we’re going to discover some more details that will better inform how to design the more complex scenarios for generalized use…in the meantime it’s easy to customize when you need to.

holyjak18:05:52

Thank you! As always, I see you have thought about this wide and deep (far more so than I). I'll try to keep this on mind as I am getting more experience.

holyjak18:05:06

(And we run again into the delicate midpoint between not general enough x general to the point of unusability. I certainly need more experience and thought to try to spot it...)

dvingo19:05:58

should this be getting normalized?

(defsc HabitRecord3 [this props]
      {:query [:habit-record/id :habit-record/date :habit-record/state]
       :ident :habit-record/id})

 (defsc TodayView3 [this props]
  {:query [{[:user/habit-records '_] (comp/get-query HabitRecord3)
          :top/prop1]
   :ident (fn [] [:component/id :today])})

  (merge/merge-component {}
    TodayView3
    {:top/prop1 "TOP"
     :user/habit-records
     [{:habit-record/id    #uuid "51d09fae-1f05-4935-8442-a8eea3355f9d",
       :habit-record/date  nil,
       :habit-record/state :incomplete}
      {:habit-record/id    #uuid "c5b39662-00ab-4480-a00c-c810c7bcf679",
       :habit-record/date  nil,
       :habit-record/state :incomplete}]})
=>
{:component/id {:today {:top/prop1 "TOP",
                        :user/habit-records [{:habit-record/id #uuid"51d09fae-1f05-4935-8442-a8eea3355f9d",
                                              :habit-record/date nil,
                                              :habit-record/state :incomplete}
                                             {:habit-record/id #uuid"c5b39662-00ab-4480-a00c-c810c7bcf679",
                                              :habit-record/date nil,
                                              :habit-record/state :incomplete}]}}}
if i replace the ident join with a plain join:
[{:user/habit-records  (comp/get-query HabitRecord3)}
                 :top/prop1]
it normalizes :
=>
{:habit-record/id {#uuid"51d09fae-1f05-4935-8442-a8eea3355f9d" {:habit-record/id #uuid"51d09fae-1f05-4935-8442-a8eea3355f9d",
                                                                :habit-record/date nil,
                                                                :habit-record/state :incomplete},
                   #uuid"c5b39662-00ab-4480-a00c-c810c7bcf679" {:habit-record/id #uuid"c5b39662-00ab-4480-a00c-c810c7bcf679",
                                                                :habit-record/date nil,
                                                                :habit-record/state :incomplete}},
 :component/id {:today {:top/prop1 "TOP",
                        :user/habit-records [[:habit-record/id #uuid"51d09fae-1f05-4935-8442-a8eea3355f9d"]
                                             [:habit-record/id #uuid"c5b39662-00ab-4480-a00c-c810c7bcf679"]]}}}

dvingo19:05:54

I also read the note about needing initial-state when joining link queries, tried that and am still not getting it normalized

(defsc HabitRecord3 [this props]
      {:query [:habit-record/id :habit-record/date :habit-record/state]
       :ident :habit-record/id
       :initial-state {}})

    (defsc TodayView3 [this props]
      {:query         [{[:user/habit-records '_] (comp/get-query HabitRecord3)}
                       :top/prop1]
       :ident         (fn [] [:component/id :today])
       :initial-state (fn [_] {:user/habit-records (comp/get-initial-state HabitRecord3)})})

holyjak19:05:31

I am not surprised that it does not work. Why do you need a join query???

dvingo19:05:50

i'm just debugging that issue from before of the missing props with nils

dvingo19:05:09

(merge/merge-component {}
      TodayView3
      {:top/prop1 "TOP"
       [:user/habit-records '_]
                  [{:habit-record/id    #uuid "51d09fae-1f05-4935-8442-a8eea3355f9d",
                    :habit-record/date  nil,
                    :habit-record/state :incomplete}
                   {:habit-record/id    #uuid "c5b39662-00ab-4480-a00c-c810c7bcf679",
                    :habit-record/date  nil,
                    :habit-record/state :incomplete}]})
i found that this works

dvingo19:05:51

but i'm guessing top level ident joins have their own code path in the merge logic, that's not present in merge-component

holyjak19:05:14

Question: in a :global-error-action I set :ui/global-error and I would like to reset it upon the next successful remote call but there is not corresponding :global-ok-action . Is the only way to set also :default-result-action!, copy and paste the default result action and modify it to remove :ui/global-error if the call has not failed?

tony.kay20:05:01

@danvingo link joins in queries are not meant for merge or load…they are a tool to jump back to the root of the client db. How they behave with merge-component is considered “undefined” as far as I’m concerned. The keys at root are either table names, or special “root level” keys. Root is just special. To be honest, if I had to redesign it today I’d make root require an ident, and there would be nothing but tables in the db…but I didn’t invent the original format. Then there’d be no need for link queries, because everything would have a real ident. You can adopt that style by simply making root be a boilerplate dummy that just does a join to your “real root” that has an ident.

tony.kay20:05:52

@holyjak If you want an “automatically clear it” behavior, then yes: you’d plug that in there.

holyjak20:05:47

Ok, thank you! For the sake of symmetry with mutations (and a simpler solution of my problem), wouldn't it be good to have a :global-ok-action (noop by default)?

tony.kay20:05:38

unsure and don’t have time to analyze it. A global error action is really meant as a dumping ground of last hope…so symmetry isn’t that interesting to me

holyjak20:05:24

Will re-read!

holyjak20:05:43

I feel the section does not help me. (1) It says ± that there are bugs, outages, and attacks, and you cannot do much about those. But when there is a bug, I want to at least provide an indication (and some kind of id for correlating with the server logs) so that the user knows something went wrong and s/he needs to talk to the customer support and has something to tell them. The section does not really address that. (2) There is a demo of adding an explicit error-action and load! :fallback but I want a global handler so that no error happens silently, with just an empty data without any indication there was an error. I wouldn't think that this use case is so rare...

tony.kay22:05:42

It is not rare: it is a small amount of code: you tell the user…the OK button on that dialog clears the error. Done. No Fulcro additions needed.

holyjak18:05:30

Hm, I haven't thought of that. Smart :-)

dvingo20:05:21

thanks Tony, I don't have a reason to use link joins, so i'm fine just using normal joins, was just playing with the merge and wasn't sure why it didn't work.

dvingo20:05:31

@holyjak FOUND IT.. for some reason the merge algo was marking fields as merge/not-found even though they are being queried for.. super frustrating.. I found it by logging in pre-merge and saw this:

{:task/id #uuid "a26a3887-a968-4348-901a-3915d1edb280",
  :task/description "task 1",
  :task/duration :com.fulcrologic.fulcro.algorithms.merge/not-found,
  :task/scheduled-at
  :com.fulcrologic.fulcro.algorithms.merge/not-found}}
and confirmed that it's the case by augmenting those fields:
:pre-merge     (fn [{:keys [data-tree]}]
                    (merge
                      data-tree
                      {:task/duration
                       (merge/nilify-not-found (:task/duration data-tree))
                       :task/scheduled-at
                       (merge/nilify-not-found (:task/scheduled-at data-tree))}))
i'm super confused why these are being marked not found, because they are in the query and the server is responding with those keys in the map, but their value is nil....

holyjak20:05:40

Well, look at where merge/not-found is set. I would not found it surprising it this was used for nil values - nil means no data so you could say that indeed no data was found for this key 🙂 Of course, your interpretation of nil is obviously different.

dvingo21:05:47

yep, it is treating nil as "missing" https://github.com/fulcrologic/fulcro/blob/f55d7a660045bca83c3258e8e0112c54665e64c4/src/main/com/fulcrologic/fulcro/algorithms/merge.cljc#L90 annyywayy. Now that I know this is happening I can design around it

dvingo21:05:37

I would have probably never noticed this was happening except that I wrote a helper that prints out all the props for a component... I'll just use the query and print all the props that way.

dvingo20:05:18

Here is the query and the reply network reply:

:task-record/task-id: {~:task/id: "~udebdf9e8-95e9-46e1-92b6-d5ce0ef42d9a", ~:task/description: "task 1",…}
~:task/description: "task 1"
~:task/duration: null
~:task/id: "~udebdf9e8-95e9-46e1-92b6-d5ce0ef42d9a"
~:task/scheduled-at: null

dvingo20:05:23

I'm so confused. the book even says this isn't supposed to happen: The general merge operations all support the option `:remove-missing?`. This option defaults to false

holyjak20:05:36

:remove-missing? is something else, it is not about nil handling but about removing stuff from your pre-existing state.

dvingo21:05:35

ahhh ok then. so i think it's in the sweep performed in the merge algo

dvingo21:05:58

thanks for the pointers btw 🙂

tony.kay22:05:36

@danvingo merge is tricky business. The book describes the decisions behind how it works. In the case of nil values: isn’t that exactly what a server would return if the value wasn’t found?

dvingo22:05:24

totally agree - I guess I was imagining "nothing" being the absense of the key in the map, but nil could also mean that. The logic from the UI's point of view is the same - a missing value. So I'm back to making progress - just went down a rabbit hole of merge/load trying to figure out what was happening. Now I know 🙂

tony.kay22:05:51

NULL in sql means “no value”, and nil is not storable in Datomic

tony.kay22:05:01

so, why should I consider nil a value that is found?

tony.kay22:05:23

That indicates that any value currenlty in app state under t hat key should be removed

tomjkidd22:05:07

@tony.kay Is it silly to want to use fulcro’s object-graph capabilities as its own standalone EQL compatible db? I am spiking on some work where using re-frame is a constraint, but I really would like to have an object graph to draw from that handles data merge and normalization. I see you’ve achieved it, and wanted to just leverage it to the extent possible

tomjkidd22:05:24

https://github.com/tomjkidd/hello-shadow/pull/1 is PR with a rough sketch of where I’m going

tomjkidd22:05:59

(also, this is just experimental throwaway code, where I’m doing things like displaying an auth token which obviously shouldn’t be part of a real app. Just trying to use available data to play around)

tomjkidd22:05:51

app.graph is the namespace that tries to provide the facade

tony.kay22:05:08

@tomjkidd I have had similar musings: the rendering part of Fulcro is a pretty thin layer. That said, the important bit is that the queries and idents are co-located on components. That’s key. I don’t know the internals of re-frame, so I really can’t advise you on how you’d integrate it’s UI control with Fulcro…but it certainly seems possible.

tomjkidd22:05:31

Yeah, I took what I saw in the tests (where Scoreboard and Score were defined), and just used the co-location to provide a way to tell how the graph is connected. It is really nice!

tony.kay22:05:41

Look at configure-component!. That is the Fulcro code that generates a component from a map of options. At the end of the day, a Fulcro component is a react class with some extra stuff (like component options) tacked onto it.

tomjkidd22:05:57

I’ll take a look. I also wanted to say thank you for all the code, and just as importantly, the documentation you have provided in the concepts that are used. Having run into the sharp edges of vanilla om.next, I truly appreciate that you’ve dedicated so much time to helping make the ideas more accessible.

tony.kay22:05:52

You aren’t going to get much mileage unless you reify the graph on the components so you have query fragments and idents…but the core logic for db handling is just db->tree and tree->db, but they do assume you’ll be using a query that is annotated with components that have the idents for normalization. That said, you could make tree->db work simply by providing a properly-annotated query…the metadata at each level has to have a :component, but all it needs to respond to is a request for idents.

tony.kay22:05:04

The approach with the “most leverage” is going to be somehow making all of Fulcro’s facilities available, and handing just the rendering over to re-frame…plugging in re-frame as a replacement renderer (which Fulcro makes pluggable) might be a tractable approach.

tony.kay22:05:15

but of course re-frame attempts to do UI refresh though that whole subscription thing…so I feel like you’re doing double-duty: writing the info on the components that Fulcro needs to know what you want to fetch, then re-telling re-frame how to get it.

tomjkidd22:05:56

I’ve kept to not actually using the components for rendering at all

tony.kay22:05:25

a watch on the state atom or something???

tomjkidd22:05:34

I really am subscribing to a logical “page”, and writing out the eql query for the entire page at the top level to make it so that everything at the component level is simple

tomjkidd22:05:00

The page implies a query, and then it is just pure passing props from the page down

tony.kay22:05:07

but you still need each “level” of the query to have a component associated with it, or you lose the tree normalization

tony.kay22:05:53

well, then it really is simple I think: Treat defsc as a way to define a query/ident. Do the normal fulcro structuring of the data tree. You need a fulcro-app to be running, but you never bother to mount it (you can just initialize it).

tony.kay22:05:10

then loads, mutations, etc etc etc will all work and update the db

tony.kay22:05:19

which is just an atom

tony.kay22:05:37

and I guess you just use that atom as Re-frame’s source db?

tomjkidd22:05:41

I had actually just stuffed the value to use with fulcro’s capability into re-frame’s db as :object-graph, and not as an atom

tony.kay22:05:03

but isn’t re-frame’s thing to use an atom?

tomjkidd22:05:04

I just have the current materialized graph, and was using just the pure functions to get a new one

tomjkidd22:05:27

Yeah, the whole db is an atom, and I put a snapshot of the object-graph into it

tony.kay22:05:45

why not just use (::app/state-atom fulcro-app) ?

tomjkidd22:05:22

I hadn’t introduced the fulcro-app into the mix at all because I was curious if I could just create the necessary components in isolation to tell how entities were connected, and start with just an empty hash-map and use the re-frame events to introduce novelty into the object-graph via merge/merge-component

tomjkidd22:05:39

Part of this was because I wasn’t sure how much/little I would need to integrate with fulcro. Even just the ability to manage a bare object graph is useful to me

tony.kay22:05:47

well, you’re missing a lot of stuff going that route

tomjkidd22:05:09

Yeah, that is why I thought doing it might be silly 🙂

tony.kay22:05:10

in fact, Fulcro Inspect watches the atom, so you’d get extended tool support that way as well

tony.kay22:05:42

no, I’m saying that the experiment would be way more useful if you let Fulcro be there as a data manager, doing all but render

tomjkidd22:05:43

I had figured it’d probably break if I just started an app, knowing it wasn’t actually being used

tony.kay22:05:53

you’d stiull get re-frame’s reducers, right?

tony.kay22:05:06

Fulcro is meant to be runnable headless

tomjkidd22:05:42

Alright, I honestly don’t have enough context on those aspects, but it gives me some targeted areas to explore

tony.kay22:05:21

so, I think it’d be fun to try