Replace deprecated use of getErrors with onError

[bensalilijames]

[email protected] has a breaking change removing the deprecated getErrors method (see graphql/graphql-js#2130). This PR replaces it with the supported onError method specified in the constructor.

TODO:

• Update CHANGELOG.md* with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

[folke]

Would be great to have this one merged. Other packages in my monorepo work with 15.0.0, which makes codegen fail. Sticking to an older version of graphql works for now, but would still be great to be able to upgrade.

[simon2k]

Great to see the PR 🎉 It would be great to merge this PR to unlock upgrading the graphql package to 15.x.

[juliovedovatto]

Friendly bump @JakeDawkins

Is there any chance to approve this PR to unlock upgrading the graphql package to 15.x? 🙏🏻

[simon2k]

@JakeDawkins @trevor-scheer @martijnwalraven Is there any sign that this could be merged after resolving the conflicts?

[simon2k]

Thanks, @benhjames for resolving the conflicts! 🙌

[trevor-scheer]

Caveat: consider this a drive-by comment for now since it's information I don't want overlooked but I'm unable to go too deep on this right now.

Given the current dependency range:

apollo-tooling/packages/apollo-language-server/package.json

Line 37 in cd56955

"graphql": "14.0.2 - 14.2.0 || ^14.3.1",

And the release milestone for the related PR whose feature this PR aims to make use of (v14.5.0):
graphql/graphql-js#2074

I think this is a breaking change for projects that depend on apollo-language-server, since it will need to update its graphql dependency to at least ^14.5.0 || ^15.0.0.

Due to the nature of the graphql package and the fact that it requires only a single instance of itself in the dependency tree, upgrading the dependency on our end may force a dependency upgrade for our users as well that have this package installed within their projects (either directly or transitively).

For some additional context on what I've just mentioned, you can take a dive into the history of my past struggles with graphql's versioning and the apollo-tooling repo in (at least) the following PRs and issue:
#1291
#1010
#624
#629

[bensalilijames]

Thanks @trevor-scheer, that's a good point!

I think I can see two primary ways forward here:

• Wait for a major release of apollo-tooling, or
• Adjust this PR so that the behaviour continues working for pre-14.5.0 graphql versions, as well as post-14.5.0. I think we should be able to check typeof context.getErrors === 'function', and if so, then use the old behaviour. Does that sound like a reasonable plan?

[juliovedovatto]

Thanks for the complete and reasonable reply @trevor-scheer

May I add my two cents in on this?

I believe it would be the best to provide backwards compatibility, like you suggested @benhjames.

[trevor-scheer]

@benhjames I fully agree with the second option and I'd feel comfortable landing that change. Some comments around that logic explaining why it's necessary and what we can clean up in the next major release would be much appreciated.

I'm thinking we may want to expand the graphql version to include v15+ (14.0.2 - 14.2.0 || ^14.3.1 || ^15.0.0) in both the language server and the CLI package in order to allow consumers of the apollo package to install graphql@^15.0.0 in their projects without ending up with multiple versions of graphql in their node_modules folder (which generally proves to be problematic).

This seems like an acceptable change with the caveat that apollo may not provide support for v15 features (i.e. interfaces implementing interfaces) but it would permit it and make use of it when installed in the parent project. My only hesitation for opening the door to v15 would be to ensure that we're not broken by the list of breaking changes here: https://github.com/graphql/graphql-js/releases/tag/v15.0.0. I do think that's something we should tackle, but probably in a separate PR whose scope is v15 preparedness.

cc @abernix

[juliovedovatto]

@benhjames

[trevor-scheer]

@benhjames Update: either today or tomorrow I'm going to open a v15 compat PR. You should feel comfortable making the changes we've talked about, I'll be addressing any other potential issues I can find. Thanks!

[bensalilijames]

Thanks again @trevor-scheer, I've added the backwards-compatible condition and comments explaining what's going on.

[dstreet]

@trevor-scheer Will this change land ahead of the v15 compat PR you mentioned, and if so do you have an estimated timeframe for when this will be published to npm?

[trevor-scheer]

Hey everyone following along, little delay on my end for this but I'll be picking up work on the backcompat PR today.

@dstreet I'd like to land mine first due to the previously mentioned desire to expand the range of the graphql version with the introduction of the change that this PR brings.

[trevor-scheer]

@benhjames those changes are perfect, thank you! This is ready to land pending the changes I intend to introduce.

[trevor-scheer]

Checking back in, have been dealing with some setbacks (particularly, wading through repo infrastructure issues dd46903) as well as some other prioritized work. This is still on my radar. Apologies for the delay!

Worth mentioning that @abernix was way ahead of his time. As I've nursed the repo and his branch back to health, it seems the onError work has already been addressed in his WIP PR here: #1743

I'll likely end up closing this PR in favor of that one, but I'll communicate that either way. Please feel free to follow along over there as well, and thanks again!

[bensalilijames]

Sounds great, thanks! I should have looked harder before opening this...! 😅

Feel free to close this PR in favour of #1743.

[trevor-scheer]

Thanks for your understanding @benhjames - for what it's worth, I was also unaware or I would've pointed to it sooner!

For anyone looking for an update, please see my comment on the related PR here:
#1743 (comment)

There are a couple one-off builds that you're welcome to try out (and feedback or confirmation would be much appreciated!).

P.S. I'm going to leave this PR open until we land the other one because there are a number of people subscribed to this one and I'd like to continue keeping them looped in over here as well.

[trevor-scheer]

Update: The releases associated with this change can be found on the next dist-tag for the following packages.

• [email protected]
• [email protected]
• [email protected]

To install the CLI from the next dist-tag, for example:
npm i -D apollo@next

I'll be closing this PR in favor of the previously mentioned PRs. Thanks again @benhjames for the help with this!

Please try them out and provide feedback on the release PR #2032. Barring any issues I'll release this officially before the end of the week.