Clarify Contribution Policy

[divanise]

Hi!

First, let me add my thanks to everyone else's for the great project!

For background, #333 is a problem for me and I'm not entirely happy with the custom makeServer workaround suggested there. I also completely understand that you're reluctant to change the existing behaviour, so I'm looking into making them myself in a fork.

As a first step, I've added a number of tests to document the existing behaviour of how errors are transported from the subscribe/resolve functions to the client. I'd like to contribute the tests back, but it's not at all clear to me how to start that process.

Story

As a developer who forked the repo,

I want to know how to discuss contributing my changes back upstream,

So that others users of the project can benefit.

Acceptance criteria

• The contribution guidelines have a section dedicated to what to do if you already have code you'd like to discuss contributing.
  • If that involves directly opening a pull request, explicitly mention that that is OK. (A lot of projects don't appreciate drive-by pull requests.)
  • If an issue should be filed, either provide a new template for contributions or clarify whether a contribution should use the bug report or user story templates.

[enisdenjo]

Hey there, thanks for the contribution interest!

Regarding #333, it is an issue with graphql-js and not graphql-ws itself. See graphql/graphql-js#1539 and graphql/graphql-js#4001. Also, the reason why graphql-ws doesn't offer built-in handling of internal errors is because there's no one-glove-fits-all. Errors are important and should be handled with care, exactly to the needs of the user - not the library author.

Regarding contributions, aside from what's stated in the contributing doc, I have no preference. You're free to open a PR or an issue, or both. IMHO, if you have a PR handy, I don't think there's a need to additionally have an issue open. All the necessary discussion can take place there.

[divanise]

Those fifteen lines of code don't properly handle iterator cancellation. That's the first approach I tried, and when I used a version of that wrapper (without the try/catch block, so I was just testing that the wrapping worked) on the full test suite for this project, several of the cancellation tests failed -- at least some of the failures were because the wrapper in your recipe doesn't return true from isAsyncGenerator when wrapping a generator.

And you're very welcome! I was hoping that the new callback I added would be interesting to you, but I understand where you're coming from and respect your decision.

[enisdenjo]

when I used a version of that wrapper (without the try/catch block, so I was just testing that the wrapping worked) on the full test suite for this project, several of the cancellation tests failed

Hmm, are you sure you did it as per the recipe? I did the recipe change internally and run the full test suite with all tests passing.

[divanise]

On the one hand, I skimmed over your link a little too quickly, sorry about that. My original test involved creating a new iterator to wrap the old one rather than modifying the old one in place.

However, using the same approach as your recipe on v5.15.0 still fails one test for me:

diff --git a/src/server.ts b/src/server.ts
index ff116a7..6771a7e 100644
--- a/src/server.ts
+++ b/src/server.ts
@@ -800,7 +800,14 @@ export function makeServer<
                 execArgs,
                 operationResult,
               );
-              if (maybeResult) operationResult = maybeResult;
+              if (maybeResult) {
+                operationResult = maybeResult;
+              } else if ('next' in operationResult) {
+                const originalNext = operationResult.next;
+                operationResult.next = async () => {
+                  return await originalNext();
+                };
+              }
 
               if (isAsyncIterable(operationResult)) {
                 /** multiple emitted results */

 FAIL  src/__tests__/server.ts (11.823 s)
  ● Subscribe › should execute the live query, "next" multiple results and then "complete"

    Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error:

[enisdenjo]

That is because the this gets polluted there. You have to bind the original next method.

- const originalNext = operationResult.next;
+ const originalNext = operationResult.next.bind(operationResult);

[divanise]

Ah, got it. Thanks!