Fork me on GitHub
#pathom
<
2020-01-09
>
fjolne15:01:17

Hi there! I’m wondering what is the recommended approach for conditional dependencies on attributes. Idiomatic approach seems to be not returning the attribute if it’s absent:

(defresolver test-x [_ _]
  {::pc/input  #{}
   ::pc/output [:x]}
  {})

(defresolver test-y [_ _]
  {::pc/input  #{:x}
   ::pc/output [:y]}
  {:y true})
But that would throw an error when trying to access :y (which might be fine to manually elide, but then we need to distinguish real errors from such ones). The other approach would be to return some absent value (e.g. nil), but that doesn’t seem idiomatic and requires more handling for each case:
(defresolver test-x [_ _]
  {::pc/input  #{}
   ::pc/output [:x]}
  {:x nil})

(defresolver test-y [_ {:keys [x]}]
  {::pc/input  #{:x}
   ::pc/output [:y]}
  {:y (boolean x)})

wilkerlucio16:01:55

@fjolne.yngling pathom assumes the following: • if an attribute is not returned, this means "I don't know to get that here, but other resolver may be able to", so other resolvers will be tried if that attribute is required • if you return nil, them its assume nil is the final value, so other resolvers will not be attempted

wilkerlucio16:01:20

the situation you talk, seems more a problem of optional input than output, because I guess you wanna run test-y even if x is not available, is this correct?

fjolne16:01:24

@wilkerlucio thanks for the quick response, I understand what you’re saying, but I have the opposite case: I don’t want to run test-y if there’s no :x. For now I’m using elide-special-outputs-plugin to sweep ::p/reader-error, but it seems more appropriate if reader would return ::p/not-found if it couldn’t resolve transitive deps for :y in my case.

wilkerlucio16:01:25

oh, it should be not-found in this case, you are seeing :x with reader-error?

fjolne16:01:28

I’m seeing reader-error for [:y] tx, and not-found for [:x] tx

wilkerlucio16:01:47

ok, that sounds more like a bug, :y should be not found

fjolne16:01:05

I’ve just checked that it’s actually not-found for parallel-reader, but error for reader2

fjolne16:01:25

and I switched to reader2 because parallel-reader accidentally hang forever in some weird cases

wilkerlucio16:01:25

sorry the mess, can you open an issue for it? I can check it out at some point this week

souenzzo16:01:15

Easy single sexp reproduce

(let [register [(pc/resolver `x1
                             {::pc/output [:x1]}
                             (fn [env input] {:x1 nil}))
                (pc/resolver `x2
                             {::pc/output [:x2]}
                             (fn [env input] {}))
                (pc/resolver `y1
                             {::pc/input  #{:x1}
                              ::pc/output [:y1]}
                             (fn [env {:keys [x1]}] {:y1 (pr-str x1)}))
                (pc/resolver `y2
                             {::pc/input  #{:x2}
                              ::pc/output [:y2]}
                             (fn [env {:keys [x2]}] {:y2 (pr-str x2)}))]
      parser (p/parallel-parser {::p/plugins [(pc/connect-plugin {::pc/register register})
                                              p/error-handler-plugin]})
      env {::p/reader [p/map-reader
                       pc/reader2
                       pc/open-ident-reader]}]
  (async/<!! (parser env [:y1 :y2])))

souenzzo16:01:17

Output with pathom 2.2.8

{:y1 "nil",
 :y2 :com.wsscode.pathom.core/reader-error,
 :com.wsscode.pathom.core/errors {[:y2] "class clojure.lang.ExceptionInfo: Insufficient resolver output - {:com.wsscode.pathom.parser/response-value {}, :key :x2}"}}

fjolne17:01:00

@U2J4FRT2T thanks, added to the issue

wilkerlucio17:01:30

I see @U2J4FRT2T just replied on that, can you confirm its the same thing for you?

souenzzo17:01:49

It's a printing loop issue, like (let [a (atom nil)] (reset! a a))

Björn Ebbinghaus18:01:52

What does this have to do with printing?

souenzzo18:01:27

it has (many) recursive references (due atoms) When clojure tryies to print it, it thows a stackoverflow error. but when you do things like (keys (parser ...)) , it will not try to print all the structure, just the keys. So, pathom is returning as expected.

souenzzo18:01:31

maybe we can add a behavior to pathom always dissoc ::p/env key from map before return. It can "fix" this issue, but I don't think that we want this behavior

Björn Ebbinghaus18:01:44

I understand that printing this recursion leads to the StackOverflow in this case. But why should this recursive reference even be in the result? Mutating without a query is fine. And updating the env in a mutation is fine as well. So pathom should not fail in this situation. Or do I misunderstand something here?

Björn Ebbinghaus18:01:11

Maybe "fail" is the wrong word here.

Björn Ebbinghaus18:01:30

These two queries should both have the same result, but they don't

(my-mutation {})
{(my-mutation {}) []}

=> {:my-mutation {}}

wilkerlucio18:01:12

thanks for the clarification @U4VT24ZM3, I believe up to Friday I'll have some spare time, so I can check on all these issues in one go, thanks for the pacience on this one

Björn Ebbinghaus18:01:48

No hurry. The issue is open for a month and the workaround is trivial. I forgot about the issue myself and the comment from @U2J4FRT2T reminded me.

Ben Grabow19:01:53

I've skimmed through the Pathom docs but I haven't noticed any mention of subscriptions. Is there any support for server-push subscriptions in pathom or is it just pull-based queries? https://graphql.org/blog/subscriptions-in-graphql-and-relay/

kszabo19:01:52

no support yet

kszabo19:01:03

is the closest issue we had around this, at least in the same space of a persistent connection that emits new data

Ben Grabow20:01:31

Would it help if I type up some details of my use case for subscriptions and add them to that issue? Push-based updates are a big part of my company's use case so I'd love to see a push-based EQL solution in the future.

👍 4
kszabo20:01:16

I think that would be helpful 🙂