Fix/pagination lifetimes #283
Conversation
a55aaf9 to
eb8d09c
Compare
| strategy: &RequestStrategy, | ||
| ) -> Response<T> { | ||
| strategy: &'a RequestStrategy, | ||
| ) -> Response<'a, T> { |
There was a problem hiding this comment.
I'm a little confused by the lifetimes here - why is tying the Response lifetime to client lifetime needed? Looks like in the function body the client gets cloned anyway, so could that client just be moved into the future so that no lifetime is needed?
There was a problem hiding this comment.
You are probably correct in questioning this. The PR is a little sloppy at the moment as it took a fair bit of experimentation. Moving it is probably more idiomatic.
|
I tried messing around with the lifetimes a bit to avoid the issue mentioned in the comment above but didn't really get anywhere. Since we end up cloning the pagination params anyway, what about just leaving the EDIT: some more general thoughts/ideas below |
|
Generally a bit wary of the added lifetime parameters, with the example in the comment around I messed around with this code for a bit - some other potential ideas:
|
|
I agree that the lifetimes definitely complicate things significantly. The idea of serializing the params immediately to a JsValue is one I hadn't considered and would simplify the types... I was trying to avoid having to special-case the codegen as much as possible so I'll have a think about this a little more, TY! |
eb8d09c to
5a7a8a1
Compare
|
Closing this since it is stalled |
Closes #246 which prevents using borrowed params with the paginator api.
Currently planning on reworking this without lifetimes by serializing the params immediately effectively making them owned.