Better handle errors from the client and detect multiple operations

[jerelmiller]

Fixes #431

Better reports errors if they originate from Apollo Client itself rather than throwing an obscure error during the manifest generation. This change also adds detects multiple operations in a single document and will report those as errors.

[changeset-bot]

🦋 Changeset detected

Latest commit: 864fdef

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

This PR includes changesets to release 1 package

Name	Type
@apollo/generate-persisted-query-manifest	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

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[glasser]

Hmm, I'm wondering why we don't have any sort of error or warning on anonymous operations rather than just not including them in the manifest and having them break...

[jerelmiller]

Hmmm, I'm not sure I follow what you're asking (or I'm reading your comment wrong). Currently we error on anonymous operations and force you to name all of them. Here is a test that shows this behavior:

apollo-utils/packages/generate-persisted-query-manifest/src/__tests__/cli.test.ts

Lines 1255 to 1277 in 930dee9

	test("errors on anonymous operations", async () => {
	const { cleanup, runCommand, writeFile } = await setup();
	const query = gql`
	query {
	greeting
	}
	`;
	await writeFile("./src/query.graphql", print(query));
	const { code, stderr } = await runCommand();
	expect(code).toBe(1);
	expect(stderr).toMatchInlineSnapshot(`
	[
	"src/query.graphql",
	"1:1 error Anonymous GraphQL operations are not supported. Please name your query.",
	"✖ 1 error",
	]
	`);
	await cleanup();
	});

Are you seeing something different in testing?

[jerelmiller]

Finally realized what I was missing which is that this check was done in a different code block that traversed the AST in fromFilepathList. For now I moved the check for the number of operations into that same codeblock. It looks like GraphQL Codegen will split those operations into separate entries in the manifest.

If we decide to include a fail-safe and would prefer we don't allow this at all, we can move the check back and update the comment explaining why we do this check in the original place rather than moving it to the function that returns the document sources.

[glasser]

Should we put the error on the second operation node as opposed to on "source" which appears in test to mean that all errors are showing up as 1:1?

[jerelmiller]

Looking back through the implementation of addError, it looks like we really don't care about node in that output and only use file and location as part of the error message, so it doesn't look like it would matter which node we use here (though I suppose we can override location to be the operation node location).

On the note about errors 1:1 with operation nodes, I considered that (and actually implemented that first), but I found this only makes sense if we include the operation name in the error output, otherwise the error just looks repetitive. Here is what something like that looks like:

  expect(stderr).toMatchInlineSnapshot(`
    [
      "src/mixed.graphql",
      "1:1  error  Cannot declare multiple operations in a single document. Please remove the 'TestQuery' operation or move it into a different document.",
      "1:1  error  Cannot declare multiple operations in a single document. Please remove the 'TestSubscription' operation or move it into a different document.",
      "src/mutations.graphql",
      "1:1  error  Cannot declare multiple operations in a single document. Please remove the 'SayGoodbye' operation or move it into a different document.",
      "src/queries.graphql",
      "1:1  error  Cannot declare multiple operations in a single document. Please remove the 'GoodbyeQuery' operation or move it into a different document.",
      "src/subscriptions.graphql",
      "1:1  error  Cannot declare multiple operations in a single document. Please remove the 'GoodbyeSubscription' operation or move it into a different document.",
      "✖ 5 errors",
    ]
  `);

vs what it is now:

  expect(stderr).toMatchInlineSnapshot(`
    [
      "src/mixed.graphql",
      "1:1  error  Cannot declare multiple operations in a single document.",
      "src/mutations.graphql",
      "1:1  error  Cannot declare multiple operations in a single document.",
      "src/queries.graphql",
      "1:1  error  Cannot declare multiple operations in a single document.",
      "src/subscriptions.graphql",
      "1:1  error  Cannot declare multiple operations in a single document.",
      "✖ 4 errors",
    ]
  `);

I went with the simpler approach because it felt pretty self explanatory on what action you needed to take, but I can be convinced differently.

Thoughts?

[jerelmiller]

@glasser would love for you to take another look here when you have some time 🙂. Let me know if the latest change alleviates your concerns.