add updateLocal method to create transaction without comitting it

[xuorig]

For #550

Adds an updateLocal method to RelayStore that creates a transaction in the Queue, but doesn't commit it.

Returns a reference to the transaction so it can be committed or rollbacked afterwards.

There weren't any tests for update so I added a test for update and updateLocal, just making sure if the correct functions on the Queue and Transaction are called.

[josephsavona]

This looks good, and thanks for the tests!

Having two such similar methods is definitely redundant though. @yungsters @wincent - thoughts on making it the default for update to not commit the transaction, and require users to call commit()? We could keep the old behavior for one release with a warning.

[wincent]

thoughts on making it the default for update to not commit the transaction

We should optimize for the most common use case, although figuring out what exactly that is is tricky. My intuition is that controlled inputs bound to mutations are actually the edge case, so the existing auto-commit behavior should be easy and the deferred-commit-managed-by-the-user behavior should be possible.

So another possible option to consider:

• Make it a single method, with the commit behavior controllable via an options object where {commit: true} is the default.

If we go with two methods, I am not sure the meaning of the updateLocal method is obvious from its name. Naming is hard, but other ideas that crossed my mind are enqueueUpdate/commitUpdate.

Finally, as for the idea of making update the only method and not committing by default, I could live with that (and it would conveniently liberate us from having to come up with decent names for two methods).

[josephsavona]

Good points. So there are three options:

• two methods, which should then both return a MutationTransaction for consistency
• one method that auto-commits by default, with an option to not commit. It returns the transaction. This means store.update(mutation, callbacks).commit() will throw (already committing) but store.update(mutation, callbacks, {commit: false}).commit() is ok
• one method, never auto-commits: store.update(mutation, callbacks).commit() or leave off the commit() call to defer.

The last option feels the least awkward (especially with a rename s/update/enqueueUpdate/), and it also makes the option to enqueue a mutation but not commit it more discoverable.

[xuorig]

the existing auto-commit behavior should be easy and the deferred-commit-managed-by-the-user behavior should be possible.

Totally agree with this, I really think the deferred commit way will be used in edge cases and probably should not be the default.

This means store.update(mutation, callbacks).commit() will throw (already committing) but store.update(mutation, callbacks, {commit: false}).commit() is ok

Which means this would make sense, but it does seem fairly awkward. I would personally probably go for this anyway I think.

Let me know what you guys decide to go for and I'll update the PR

[josephsavona]

@xuorig thanks for your patience. We chatted offline and came up with the following API:

1. Relay.Store.applyUpdate(mutation, callbacks): RelayMutationTransaction - does not commit the mutation immediately (e.g: rename updateLocal to applyUpdate)
2. Relay.Store.commitUpdate(mutation, callbacks): RelayMutationTransaction - commits the mutation immediately (this replaces the existing update)
3. Relay.Store.update(...): void - mark this as deprecated and suggest using commitUpdate.

Would you mind changing updateLocal -> applyUpdate and adding docs for this method (item 1)? We can address 2 & 3 separately (of course you're welcome to do these too if you have time). Thanks!

[xuorig]

Alright, I changed the method name to applyUpdate + updated the docs with example and some info.

Squashed + reworded commit also.

I will tackle 2 and 3 in another PR

[josephsavona]

nit: transaction: RelayMutationTransaction (also should update the above doc for update for consistency)

[josephsavona]

@xuorig awesome, thanks! just two minor nits, otherwise this looks good to go.

[xuorig]

Addressed your nits!

[josephsavona]

@facebook-github-bot import

[josephsavona]

👍

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1727015024195186/int_phab to review.