Fix error message generation

[lovasoa]

ApolloError.graphQLErrors is typed as an array of GraphQLError in typescript, but it can actually contain any value
at runtime, including null. This is even explicitly tested:

apollo-client/src/core/__tests__/QueryManager/index.ts

Lines 369 to 376 in caba6c1

	errors: [null as any],
	},
	observer: {
	next() {
	reject(new Error('Should not fire next for an error'));
	},
	error(error) {
	expect((error as any).graphQLErrors).toEqual([null]);

This PR simplifies error message generation, and prevents ever creating the string 'undefined' as an error message.

See: #10394

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

[changeset-bot]

🦋 Changeset detected

Latest commit: 0268e49

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

This PR includes changesets to release 1 package

Name	Type
@apollo/client	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

[lovasoa]

The netlify action is failing with Failing build: Failed to prepare repo, but I'm not sure this is related to this PR, is it ?

[alessbell]

This subtly changes error message generation behavior in that it sets the message to 'Error message not found.' once at the end if none of the merged graphQLErrors and clientErrors have a message property instead of appending a new "not found" for each error that lacks a message.

We evidently don't have tests for this behavior since all tests are passing, but would prefer to make this a backward-compatible change in order to get it out in a patch release.

Would you be open to adding a test case for preventing the undefined case you mentioned in the description, @lovasoa, as well as reinstating the old logic?

[lovasoa]

@alessbell , the old behavior is a bug, isn't it ? Is there any reason to have an error message that says Error message not found.\nError message not found.\nError message not found.\nError message not found.\nError message not found.\nError message not found.\nError message not found.\nError message not found.\nError message not found.\n ? I cannot imagine anything relying on this bug. Backwards compatibility is a good thing, but it shouldn't be a justification to keep this kind of problematic behaviors. Or is there something I do not see ?

[netlify]

‼️ Deploy request for apollo-client-docs rejected.

Name	Link
🔨 Latest commit	430f779

[alessbell]

Hi @lovasoa 👋 Thought I'd check in and see if you're still interested in moving this PR forward? No problem at all if not, but it would be great to land this simplification/small reduction in code here!

I agree it's hard to imagine anyone relying on that particular behavior, but we do want to be mindful of potentially breaking changes in a sensitive codepath like this, especially at a new patch version (and I have certainly seen stranger things 😅)

We can extract the generic error message into something reusable, remove the filter and in our map do something like:

isNonNullObject(err)
  ? (err.message || ERROR_MESSAGE_NOT_FOUND)
  : ERROR_MESSAGE_NOT_FOUND

With isNonNullObject imported from the same utilities folder we're currently importing isNonEmptyArray from. This will also give us a tiny bundlesize reduction of 8 bytes 🎉 Let me know what you think :)

[lovasoa]

@alessbell : done ✅

[alessbell]

@lovasoa looks great! I see you've pulled in the changes from your other PR: can we keep those separate in the interest of getting this one merged?

We can't change the behavior of isLikeCloseEvent since we need to detect both CloseEvent with a code + reason and WS errors which return an Event. If you prefer to combine them, you can take a look at bc5c766 where I've added some tests.

[lovasoa]

@alessbell , sorry, I had accidentally rebased on the wrong branch. It's fixed now

[alessbell]

@lovasoa thanks for updating that 👍

There were two newly failing tests since a PR just landed that added protocolErrors; hope you don't mind I pushed that small update here f9daa14 and added a changeset (430f779) so this PR will be mentioned in the CHANGELOG and release notes for 3.7.11.

[alessbell]

Thanks @lovasoa! A nice improvement in functionality and readability while reducing a few bytes in the generated code 🎉