kitten
commented
1 year ago Sorry for coming back to this a little late.
I don't think I see the intention here and the full solution as even feasible.
updateItem({arg}, {extraData: {someExtraData})This implies we're taking a random key fromOperationContext.updateItem({arg}, {}, {extraData: {someExtraData})This implies that we are to take this option from a new bindings option, which goes against the design decisions of the bindings, since this mixes concerns from the UI/bindings and a single exchange, i.e. Graphcache.
Anyway, I'll move on for a bit to the actual proposal; I don't think the reasoning here is necessarily justified for two reasons.
Firstly, it assumes that user-controlled data is a concern for API design in Graphcache, i.e.:
I think this is opaque to somebody looking at the function call While I think it's important to include clarity in API design, this specifically is not an API, but it's a small feature that's part of a bigger edge case. In other words, it "happens to work" and while we guarantee that it continues to do so, I see this more as an implementation detail that enables some escape hatches. tl;dr: it's an escape hatch.
Secondly, this assumes that the user has no additional knowledge at the call site, which isn't correct. In my opinion, bindings are a basically a boundary — we can think of them as crossing a UI framework to GraphQL. So, updateItem isn't what I'm focused on, but it's instead the mutation that's eventually used.
If we look at variables only used in optimistic updaters, we basically have access to variables that aren't defined by the mutation itself, which is a hint that the schema/API doesn't consume them.
Furthermore, this is already done via an added "meta object", i.e. the context that is passed to the optimistic function (or resolvers, or updaters, for that matter).
Nothing also stops us from giving it a name that indicates that it's a "private" variable, e.g. _extra or anything else that's prefixed as such. In fact, we can even give it a name that's impossible in GraphQL, e.g. extra$.
To summarise, I don't think this RFC crosses the threshold of fulfilling a need that is, as of now, unfulfilled.
To address the ergonomic reasoning here, and talk about the API design, I'll come back to it being an escape hatch. In theory, given an optimistic update, the cache often contains enough information to create a sufficient optimistic shape without extra variables.
This was further addressed by https://github.com/urql-graphql/urql/pull/3264 and https://github.com/urql-graphql/urql/pull/2616
The latter PR lies some time back but allows nested optimistic functions, which allows for more handling of nested optimistic fields without passing in extra data. The former, more importantly, now allows optimistic results to be almost fully partial. This was less obvious originally, but optimistic results now have more guarantees around what happens when they're incomplete.
I'm open to more discussion here of course, but for now I'll close this RFC. I think without some signs of this being a bigger problem (and the escape hatch not becoming a typed part of mutations anyway), I think it's best to collect more cases and samples for this before considering any further change
Summary
It is currently possible to pass in additional variables for optimistic updates, however at the point they are supplied, I believe this is quite confusing. Currently the extra args are mixed in with the args intended to be passed to the mutation:
I think this is opaque to somebody looking at the function call as there is no differentiation between mutation args and extra data.
Proposed Solution
Accept the extra data as an explicit
extraDataadditional key on theoperationContext, or as a key on a third-arg config object if you feel it doesn't belong there.