fix(execute-exchange): prune undefined variables

[fathyb]

Summary

This allows for directives to correctly validate with variables using default values.

query($count: Boolean = false) {
  users {
    # Fails if $count is not provided:
    # Argument "if" of non-null type "Boolean!" must not be null.
    count @include(if: $count)
    list { id name }
  }
}

Set of changes

• exchanges/execute (@urql/execute-exchange):
  • src/execute.ts/executeExchange(): filter undefined variables before passing them to graphql.execute()

[changeset-bot]

🦋 Changeset detected

Latest commit: 215a88c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@urql/exchange-execute	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[kitten]

Can you provide a test for this if it's not too much trouble that demonstrates this issue?
Also, on a side note we can make this fix a lot less wasteful by doing:

Suggested change

	const variableValues = operation.variables
	? { ...operation.variables }
	: operation.variables;
	if (variableValues) {
	Object.keys(variableValues).forEach(key => {
	if (typeof variableValues[key] === 'undefined') {
	delete variableValues[key];
	}
	});
	}
	const variableValues = Object.create(null)
	if (operation.variables) {
	for (const key in operation.variables) {
	if (operation.variables[key] !== undefined)
	variableValues[key] = operation.variables[key];
	});
	}

Generally, this is the style we prefer to avoid unnecessary closures, so we make it a habit throughout the codebase ✌️

[fathyb]

Thank you for the feedback! I've pushed a commit addressing both points.

Few notes:

• I needed a new query fixture, I wasn't sure what was more appropriate: update the existing ones, or create a new one. I went with updating an existing one, which required updating a few snapshots. The fixture change wasn't necessary because of the following note, I've reverted it!
• Ideally, we should call graphql.execute with an executable schema linked to mocked resolvers. It looked like it would've required a considerable amount of code, so I went with simple assertions in order to keep the PR lean.

[JoviDeCroock]

Nice work

[kitten]

Thank you so much ❤️🙌