Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merge! can't access root component #550

Closed
wilkerlucio opened this issue Dec 23, 2015 · 7 comments
Closed

merge! can't access root component #550

wilkerlucio opened this issue Dec 23, 2015 · 7 comments
Labels
Milestone

Comments

@wilkerlucio
Copy link
Contributor

(def tempid (om/tempid))

(defui Item
  om/Ident
  (ident [_ {:keys [db/id]}] [:by-id id])

  om/IQuery
  (query [_] [:label :db/id])

  Object
  (render [this]
    (let [{:keys [label]} (om/props this)]
      (dom/div nil label))))

(def item-view (om/factory Item {:keyfn :db/id}))

(defui App
  om/IQuery
  (query [_] [{:app/items (om/get-query Item)}])

  Object
  (render [this]
    (let [{items :app/items} (om/props this)]
      (apply dom/div nil
        (dom/button #js {:onClick #(om/transact! this '[(item/consolidate) :app/items])} "consolidate")
        "Items"
        (map item-view items)))))

(defn local-read [{:keys [state query]} key params]
  (let [value (get @state key)]
    (if value
      {:value (om/db->tree query value @state)})))

(defn local-mutate [_ _ _]
  {:remote true})

(def parser (om/parser {:read local-read :mutate local-mutate}))

(def reconciler
  (om/reconciler {:state  {:app/items [{:db/id tempid
                                        :label "sample"}]}
                  :parser parser
                  :id-key :db/id
                  :send   (fn [_ cb]
                            ; running this will clear the local database
                            (cb {'item/consolidate {:tempids {[:by-id tempid] [:by-id "123"]}}})
                            ; by sending the query manually like the version below make it work
                            #_ (cb {'item/consolidate {:tempids {[:by-id tempid] [:by-id "123"]}}} (om/get-query App)))}))


(defcard root-issue-card
  (om/mock-root reconciler App))

The code above demonstrates an issue when merging new data into Om.next. Running the example above you can see that when we ask to merge the new delta the database is going to be {:om.next/tables #{}, :om.next/queries nil}.

You can send the query manually, if you do so it will work as expected, on the code I found at this line: https://github.com/omcljs/om/blob/master/src/main/om/next.cljs#L1369, which leads me to believe that by default it should be getting it automatically from the :root, I believe the error is around the management of the :root component value at :state.

@akmiller78
Copy link

I noticed that if you change this example to actually use add-root then it works just fine. I haven't pinpointed the exact issue yet but when using om/mock-root I noticed that this call returns null (https://github.com/omcljs/om/blob/master/src/main/om/next.cljs#L1369):

(get-query (:root @(:state reconciler)))

It appears that the item at :root in this case neither implements IQuery or is a function so the get-query does not handle it.

@awkay
Copy link
Contributor

awkay commented Jan 1, 2016

I'm looking into this further. One note: You forgot static on Ident and IQuery. Unfortunately, this does not seem to fix it (as I hoped it would).

@akmiller78
Copy link

I reduced it even further and when just trying to access the reconciler from the repl with the mock root it errors. However using the actual root it's fine. Seems like it must be related to not providing a target.

@awkay
Copy link
Contributor

awkay commented Jan 1, 2016

Yeah, so when using mock-root, then a direct call to the factory function is used as the :root setting in the reconciler (

(swap! state assoc :root c)
), and this (which is a not-yet-rendered react element) is not compatible with get-query. The resulting component fails both the isFunction, and implements? IQuery tests in get-query.

@awkay
Copy link
Contributor

awkay commented Jan 1, 2016

As a workaround, you can use the devcards dom-node helper:

(defcard root-issue-card
  (dom-node (fn [_ node] (om/add-root! reconciler App node))))

@swannodette swannodette added this to the Beta 1 milestone Jan 2, 2016
@swannodette swannodette added the bug label Jan 2, 2016
@akmiller78
Copy link

Left a comment on the pull requests, but it seems the only reasonable solution is to remove mock root and advise the use of dom-node as @awkay mentions above. Then, a constraint could be added to the target to ensure that it's either a dom node (react) or number (react native).

@swannodette
Copy link
Member

fixed 4b0a9af

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants