Urql: Add warning in development when a mutation fn is called with an event

[morajabi]

At the end of this section, we have onClick={addTodo}. So this wouldn't work at if you try it you'll get an error from this line with this error pointed out in this issue.

Basically, the event object gets passed in and GraphQL.JS thinks it is the variables object (which should not because I already provided the variables, hmm!) and thus it cannot convert it to JSON since it is a circular object.

[blainekasten]

I wonder if for the simplicity of the API sake, we could analyze each mutation argument. And if it's event-like, we ignore it. I like the idea of being able to do onClick={addTodo}.

[morajabi]

@blainekasten I agree, but as I said, when you have already passed the variables in before, event should not get through in the first place 😉 It seems it can be kinda a bug too. But for mutations without variables, your idea makes sense, but it is already avoidable by () => addTodo() but I think we'll get this issue over and over again here so for that sake @kenwheeler might like this idea of analyzing objects.

[blainekasten]

Shouldn't be too hard. I think at this line: https://github.com/FormidableLabs/urql/blob/a98986406e92f1a5b68df2e7455b77fa39511c94/src/modules/client.ts#L168

We would just loop the variables and rebuild a map while filtering out any object that matches an Event heuristic. Honestly I think this pseudocode would suffice:

const filteredVariables = {};
const { variables } = mutationObject;

Object.keys(variables).forEach(key => {
  const variable = variables[key];

  if (typeof variable === 'object' && typeof variable.preventDefault === 'function') return;
  filterVariables[key] = variables[key];
});

const body = JSON.stringify({
   query,
   variables: filteredVariables,
});

[kitten]

@blainekasten I think this could be great as a warning in dev. We can throw an error in prod potentially? Or just ignore it there and remove the check in prod. Are we committed to introducing this, @kenwheeler?

Also, wouldn't this check be enough?

const isEvent = x =>
  typeof x === 'object' && (
    (typeof x.nativeEvent === 'object' && x.nativeEvent instanceof Event)
    || x instanceof Event
  );

[dennisja]

@kitten Is this being worked on?

[kitten]

v1 is released now which means this issue was resolved / is out-of-date 👌

Some docs and a proper announcement for v1 are still in progress in pending. It's completely usable so far but the API has changed, so I have to ask for some more patience, but everything will be ready quite soon ✌️