Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: change arg format of update mutations #45

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shrouxm
Copy link
Member

@shrouxm shrouxm commented Jul 26, 2023

the old format encouraged sending unnecessary data and did not guard against bugs from sending extra data

Description

Doing e.g. a site update mutation before would look like:

dispatch(updateSite({id: site.id, name: newName}));

but could look like this if e.g. we had a site object that already had the new data on it, which is much more tempting

dispatch(updateSite(site));

the second one is bad for a few reasons:

  • if we send extra fields in the GraphQL mutation, it will cause a backend error, and the site model might be a superset of the site update mutation args
  • if we send fields that are not being updated, we could accidentally revert a simultaneous update to a different field by a different user that hadn't arrived to our device yet. we are also sending unnecessary data over the wire
  • neither of the above errors can be caught by unit tests and we don't have comprehensive integration/e2e tests
  • we could guard against the first thing by restricting the fields within the updateSite method, but that doesn't solve the second problem and restricting the fields is finnicky and we have to remember to update it all the time

if we simply remembered to never do something like the second thing it would be ok, but there's nothing prevent us from doing it except our brain. this PR creates a helper type and validation function to make the args for updates look like this:

dispatch(updateSite({id: site.id, update: {name: newName}}));

this isn't perfect, it would still e.g. admit the following:

dispatch(updateSite({id: site.id, update: site}));

but i'm hoping that will be a less intuitive thing to do, and also we can do a runtime client-side check with this format so it'll be easier to catch in tests. there is unfortunately no way to give update a type that says "reject arguments that have an id field" as far as i know in TypeScript so it can't be a compile-time check

does this seem like a worthwhile thing to do? or should we just remember not to do

dispatch(updateSite({...site, name: newName}));

?

the old format encouraged sending unnecessary data
and did not guard against bugs from sending extra data
@shrouxm shrouxm force-pushed the feat/update-mutation-format branch from 503eee7 to 63b90f2 Compare July 26, 2023 17:04
@josebui
Copy link
Contributor

josebui commented Jul 26, 2023

@shrouxm This makes sense to me, the only thing that doesn't completely map to all the cases for the Terraso web app is that we are not always using the id as the identifier to execute a mutation there are other identifiers like the slug or the email in the case of a user that can be used as an indentifier, so if that identifier could be generic I think that can capture more use cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants