Allow async edge connections #1829
Conversation
This will allow the edge connection function to return a promise and still work the same.
| first(limit: number, cursor?: string) { | ||
| this.query = this.query.first(limit, cursor); | ||
| this.query = Promise.resolve(this.query).then((query) => | ||
| query.first(limit, cursor), | ||
| ); | ||
| } | ||
|
|
||
| last(limit: number, cursor?: string) { | ||
| this.query = this.query.last(limit, cursor); | ||
| this.query = Promise.resolve(this.query).then((query) => | ||
| query.last(limit, cursor), | ||
| ); |
There was a problem hiding this comment.
hmm. what's the usecase?
is this idiomatic TS?
do we want to use Promise.resolve here? doesn't seem ideal
There was a problem hiding this comment.
what's the usecase?
I have a custom edge query where the user can pass in a location string and I fetch the lat/lon before running the query, so I need that method to support returning a Promise.
is this idiomatic TS?
Not entirely sure what you mean, but it's normal JS here. This is also fully typed in TS.
do we want to use Promise.resolve here? doesn't seem ideal
I couldn't think of a better way to do it since EdgeQuery.first and EdgeQuery.last are functions that mutated the object. I don't think it's unideal to use Promise.resolve here. If you call it more than once it's a no-op, and if you call it on the non-promise it's just an immediate return.
There was a problem hiding this comment.
basically we have a non async function here which resolves a Promise and doesn't play well with the event loop.
is this worth a breaking change or a different API for this case?
| @@ -30,7 +32,7 @@ export class GraphQLEdgeConnection< | |||
| TEdge extends Data, | |||
| TViewer extends Viewer = Viewer, | |||
| > { | |||
| query: EdgeQuery<TSource, Ent, TEdge>; | |||
| query: MaybePromise<EdgeQuery<TSource, Ent, TEdge>>; | |||
There was a problem hiding this comment.
might be simpler to take a query here and update everything to work on Promise
so this.query = Promise.resolve(query) and everything below uses that same query?
what happens when you create multiple Promise.query on the same instance multiple times? we end up calling the function multiple times right?
I think you need a test for this case that handles when it's passed a Promise instead of just the function
there's also more code that's going to need to be updated since the value of query is different now
see https://github.com/lolopinto/ent/actions/runs/10712869234/job/29703982341?pr=1829
There was a problem hiding this comment.
so this.query = Promise.resolve(query) and everything below uses that same query?
Oh that's a good idea.
what happens when you create multiple
Promise.queryon the same instance multiple times? we end up calling the function multiple times right?
Assuming you mean Promise.resolve, If you call Promise.resolve(Promise.resolve(somePromise)), it's the equivalent of doing await (await somePromise), the outer one is just a no-op since the promise is already resolved.
I think you need a test for this case that handles when it's passed a Promise instead of just the function
👍 I'll get on that today
there's also more code that's going to need to be updated since the value of query is different now
see https://github.com/lolopinto/ent/actions/runs/10712869234/job/29703982341?pr=1829
Thanks. I'll do a more thorough check to make sure there aren't any others as well.
There was a problem hiding this comment.
what happens when you create multiple Promise.query on the same instance multiple times? we end up calling the function multiple times right?
Referring to Promise.resolve(query) N times. Does it end up running the underlying await function multiple then? Do we need some kind of memoize? Or we just leave that out of scope here?
There was a problem hiding this comment.
It does not end up calling it multiple times, once a promise is resolved, calling resolve on it again just returns the resolved value immediately.
There was a problem hiding this comment.
But actually with your suggestion of making this.query always a promise, I'm no longer calling Promise.resolve multiple times anyways. 👍
There was a problem hiding this comment.
Ok, then in this case doing the Promise once at the beginning seems like a good thing since previous implementation would have created a new Promise.resolve at multiple call sites?
There was a problem hiding this comment.
Yep. Just updated the PR to the one Promise.resolve.
Also added a test for async query functions.
This will allow the edge connection function to return a promise and still work the same.
Fixes #1827