Help with map transducer?

[yaacovCR]

#48 (comment)

See https://github.com/yaacovCR/graphql-executor/blob/c254cdcc7254b07f583c6391d2103fe8e2761a52/src/execution/mapAsyncIterator.ts#L30-L38

which I changed to make sure the map does not hang.

...but there are a lot of failing tests now in yaacovCR/graphql-js#54

I am not so concerned right now about the tests in https://github.com/yaacovCR/graphql-executor/blob/reform-map/src/execution/__tests__/mapAsyncIterator-test.ts because I am not sure how correct that file is.

But there are functional differences it seems in the order of next/return calls vs next/throw calls in the tests here https://github.com/yaacovCR/graphql-executor/blob/c254cdcc7254b07f583c6391d2103fe8e2761a52/src/execution/__tests__/subscribe-test.ts#L889-L976 where the first passes, and the second fails -- I am not sure which should be correct, but I would think they should be the same.

[yaacovCR]

@brainkim Thanks so much for your help with this. I am not sure how much you know about graphql-executor, but for now it is a testing ground for potential PRs to graphql-js, as well as a convenient way to customize execution. I see a number of benefits to using repeaters within graphql-js, so we shall see how this goes.

[brainkim]

@yaacovCR That’s exciting work! Can you distill the misbehavior down to a single test or two? If not I will take a look at running/reading through the GraphQL executor codebase when I can (a little busy with some other things at the moment).

[yaacovCR]

The last failing test here is what I am concerned about: https://github.com/yaacovCR/graphql-executor/runs/4117850549?check_suite_focus=true#step:7:685

[yaacovCR]

And I should say it’s possible I messed up the repeater class when linting it more strictly as required by graphql js

[brainkim]

The way throw() works is slightly finicky, so I’d probably also have to see what’s going on with your changes 😣 Again, if you can distill the misbehavior down to a code snippet, I would be eternally grateful!

[yaacovCR]

I am working on a PR to this repo with the failing test/difficulties.

[yaacovCR]

Summarizing my thoughts so far, in terms of the secure stop problem raised by @jedwards1211, we have solved it for return, but not for throw. I believe this may be a limitation in async generators as well. I have confirmed that the behavior I am seeing is the same in terms of original repeater package and the one inlined into graphql executor.

Calling return () returns right away, but calling throw () is halted if the executor is awaiting some promise.

Not sure how this could be worked around in terms of the semantics of throw, I am just surprised by the difference in behavior, although perhaps that is per spec.

[yaacovCR]

Maybe we could solve this if there was another parameter to the executor, some way to detect if throw was called, so we could race that event with the call to next on the underlying iterator.

i think the fact that test passes with the graphql js native async iterator version of map is because they don’t have to ensure everything settles in call order.

[yaacovCR]

By the way, I updated graphql executor to just use actual repeaters vs the inlined version, get the same behavior, it’s not due to code edits on my part. I don’t think it’s a bug in repeaters, just further difficulty in using repeaters to get a straightforward map transducer, as mentioned above.

This is a problem in the test cases in graphql js/executor because the next underlying async iterator event is specifically not fired by the test scenario until after the throw call resolves, which it doesn’t, because it is awaiting that very same payload. In a real life scenario, the event would be fired asynchronously, and so the code would not hang.

[yaacovCR]

Come to think of it, the entire return vs throw discrepancy may be because the underlying subscription from the graphql js Simple pub sub is not a repeater either. Maybe I will retry replacing that pubsub implementation with yours prior to replacing the map async iterator implementation

[yaacovCR]

Closing, as nothing seems particularly actionable. thanks for providing a space for me to figure things out more. Would love to hear your thoughts on whether GraphQL JS should return more generator like iterators

[brainkim]

@yaacovCR

Right, so return() and throw() have slightly different use-cases. The return method will pass a return statement in at the suspended yield of the generator; the throw method will “throw” in an error at the suspended yield. Returns are non-recoverable; you can theoretically use a try/finally to yield more values, but no one should be doing that. Throws on the other hand, are meant to be recoverable, using try/catch.

So while it might make sense for returns to work in parallel with current next calls, it doesn’t make sense for throws to work in parallel, because the result of the throw should necessarily happen in call order.

To be honest, I wouldn’t worry too much about the throw operator, insofar as it really shouldn’t be used by any of GraphQL.js’s iterators.

[yaacovCR]

That's basically what I came to, put much better! Thanks again so much for your time.