Update handler names for useSubscription hook

[jerelmiller]

Closes #10116

With the addition of onError (#9495), the names of the other on* methods (onSubscriptionData and onSubscriptionComplete) are in a bit of misalignment with each other. This PR seeks to unify the names of the other two on* methods with onError to feel cohesive. This PR deprecates the onSubscriptionData and onSubscriptionComplete functions with a warning. Passing both onSubscriptionData and onData or onSubscriptionComplete and onComplete will also result in a warning explaining that only one of them will be used.

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

[apollo-cla]

@jerelmiller: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[jerelmiller]

I'm not sure what the release strategy is for this, but since onError was added for 3.7, I figured I'd start with opening this PR against that branch. Feel free to update if this feels like something that should be in a followup version.

[jerelmiller]

Should I consider updating documentation for useSubscription as part of this PR as well?

[alessbell]

I like this approach (and three cheers for first PR!! 🎉)

Do we already have a separate ticket for updating the docs? Seems like it shouldn't be a big diff so feel free to include it here or we can take care of it in a follow-up, e.g. copying the way Resolvers is marked as deprecated in the testing API docs. (Incidentally it looks like onSubscriptionComplete is currently missing.)

[alessbell]

Nit: could also have OnDataOptions extend OnSubscriptionDataOptions until the latter is removed to ensure the types don't diverge before intended.

[jerelmiller]

That's not a bad idea, though I go back and forth on this. I hesitate changing it only because we call these separately, so a divergence might be natural. In fact, one tweak I made from onSubscriptionData is that I called the option data instead of subscriptionData to better align with the updated name of the function.

It's a coin toss, so let me know if you feel strongly and I'd be happy to update.

[jerelmiller]

@alessbell I'll go ahead and include it in this PR. Doesn't look like it will be much to change it. Plus it gives me a chance to add another commit to hopefully kick off the tests again 😆 (I'm hoping me logging in fixed that).

[jerelmiller]

@alessbell looks like that fixed it! Tests are now running. Turns out there is also the Subscription component to worry about, so this gave me a chance to add tests for that as well. This should be good to go otherwise. Thanks so much for reviewing!

[jerelmiller]

I seem to be victim to some flaky tests or something 🤔 . Seems a test or two times out when running the full suite, but it seems to change each run. Any ideas on how I might be able to get these to pass? I'm able to run the full suite locally with success which makes this a tad harder to debug.

[alessbell]

I seem to be victim to some flaky tests or something 🤔 . Seems a test or two times out when running the full suite, but it seems to change each run. Any ideas on how I might be able to get these to pass? I'm able to run the full suite locally with success which makes this a tad harder to debug.

Now that I've merged #10140 (thanks for the review!) and merged main into release-3.7 (+ fixed some conflicts), I'm hoping that when you rebase/do a merge commit here the tests turn green 🤞

[jerelmiller]

@alessbell woohoo! Looks like that did it. This should be ready to go. Thanks for getting that in!

[jerelmiller]

Something I discovered when writing tests for the Subscription component is that this has a data property on it, which means accessing that data is via options.data.data. Do we feel the double data.data is too weird? This was called subscriptionData in onSubscriptionData, but I changed it to better align with the new name of the hook.

Example usage:

useSubscription(subscription, {
  onData: ({ data }) => {
    console.log(data.data)
  }
})

Thoughts on this? Are we ok with this?

[alessbell]

Sounds like a potential future refinement - I don't mind creating an issue for visibility if you haven't already 👍

[jerelmiller]

Will do!

[jerelmiller]

Added this issue in #10142

[alessbell]

@jerelmiller great! This is good to squash+merge in that case 🎉