@peeja https://github.com/compassus/compassus/commit/b63876a806bb03b3e462754136bfcd24fdf2001b

Uh oh. Now I'm getting the dreaded error: No queries exist for component path (compassus.core/ui_32146 frontend.core/ProjectsPage frontend.core/OrgListItem) or data path [:compassus.core/route-data :app/current-user :user/organizations] It appears to be because the props have the path [:compassus.core/route-data :app/current-user :user/organizations], but the indexer has indexed the component as [:compassus.core/route-data [:app/current-user _] :user/organizations 0].

I don't think my parser's doing anything wrong; the structure of its response is what comes out of db->tree

@peeja looks like a bug, but I thought we never constructed paths with links in the indexer

hrm

can you produce a minimal case?

I can try

The bug is that the indexer shouldn't have the link in its path?

I think so

@peeja actually I see the bug https://github.com/omcljs/om/blob/master/src/main/om/next.cljc#L1786

but this one might not be that simple to figure out, can you open an issue?

Sure. I've got a test written, too.

👍

https://github.com/omcljs/om/issues/773

@peeja that test is supposed to fail

that’s not the bug

Oh, then I'm misunderstanding what you were saying above

class-path != data-path

the query should contain the link

the data path maybe shouldn't

Oh. Yeah. I'm not misunderstanding, I'm just writing the test wrong. 😛

but this is a tricky one

Yeah…

Right, in other cases, it actually does match the query

So…the issue is that full-query needs to be more sophisticated?

that’s probably right

https://github.com/omcljs/om/blob/ee4c7ac33934fcd9b3ea663045ced1408dd7df24/src/main/om/next.cljc#L1980

this condition should probably flatten the links it finds

Yeah, that makes sense to me

cljs.user=> (om/focus->path [{[:a '_] [{:foo [:b :c]}]}])
[[:a _] :foo]

this is probably right

that’s the actual path of the query

so for the tweak should happen in full-query

@peeja probably worth tweaking the issue title and description, seems a bit misleading now that we have this information

Yeah, I'm trying to write a new test

BTW it’s really great you approaching all this from a new angle

I'm glad I'm not just a pain in the butt 🙂

I appreciate we finding all these bugs before beta 🙂

Huh, are there any tests that call full-query? I don't see any. Should the test be at a different level?

@peeja there are no tests for full-query, probably worth adding some

problem is that we need an assembled indexer to call it 🙂

Yeah, I'm just noticing that 🙂

Not just an indexer, but a reconciler

you can probably get away by using with-redefs for get-indexer

because you want to test at the point that we already have qs

that make sense?

How do I instantiate a component here?

(Component. #js {:omcljs$reconciler r :omcljs$path [the path you want the component to have]})

@peeja and there’s a slightly different version for the :clj branch

you can find it in the test suite

Ah, I see it now. Thanks.

Okay, updated with correct info (as far as I know) and a test: https://github.com/omcljs/om/issues/773

@peeja try applying the commits on these 2 branches and see if your issues are gone: https://github.com/anmonteiro/om/tree/om-772 https://github.com/anmonteiro/om/tree/om-773

@anmonteiro 773 works great for me. 772 still has one problem: if there's a component with that ident as its ident, and there's a component with the ident in its query, the logic short-circuits and fails to find the second one. Here's a version which finds both: https://github.com/Peeja/om/commit/23556d04f8bd1b1e62784c3aefef1b0b57226fae

@peeja yep, definitely an overlook. Thanks for the added test, I’ll use that to fix the PR

.. and ping you when it’s done!

@anmonteiro There's implementation on there too, if it looks good

will have to study it carefully

It's just a union of the two searches. There's no keyword? or ident? check, but I don't think that check is really necessary: if you supply a bogus key, you just won't find anything in the index.

@peeja yeah I’ve noticed that, and I agree

still have to look more closely because of perf reasons

Yeah, sounds good :thumbsup:

e.g. set/union might not be as performant as into with a transducer

I’ll look in a few

It probably should be, whether it turns out to be or not. If you find it isn't, that may be worth raising as a ClojureScript issue.

Oh, actually, it looks like it's almost literally into: https://github.com/clojure/clojurescript/blob/r1.7.170/src/main/cljs/clojure/set.cljs#L19-L29

👍

@peeja into with sets just works fine so no need to introduce the clojure.set dependency at all

cljs.user=> (into #{1 2} #{3 4 2})
#{1 2 3 4}

other than that the code is almost the same as yours, just s/union/into