Removed parallel execution requirement for merging fields

[rivantsov]

The main point here is to remove 'when executed in PARALLEL'; service is free to execute the request 'normally' - in any order it wants, parallel or not. But I think the field merge process should always apply.

As for the final text - suggestions are welcome

[netlify[bot]]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	3f413e4e06e34ff15dbcf8be47872d79651bdefb
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/62ea29e6f1187b00084afba8
😎 Deploy Preview	https://deploy-preview-985--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[rivantsov]

Just a question. About proper English:

When more than one field of the same response name is executed ...

is it plural or singular?! fields are many, so it's plural. But IS executed? singular. I am confused :(

[benjie]

I believe "is" is correct English here, because the noun is "field" (singular) not "fields" (plural). English is weird.

[rivantsov]

yeah, that feels weird, and that's what pushed me to switch to 'multiple fields' in the first place; non-ambiguous PLURAL

[rivantsov]

Regarding using 'response name'. There are 4 occurrences in the spec of 'response name', none of them defines it, only use it as already known. There is a term 'response key' explained and kind of defined here in 2.7 Field Alias:

https://spec.graphql.org/draft/#sec-Field-Alias

maybe we should think about switching to 'response key', here and in 4 other locations.

[michaelstaib]

@rivantsov I also was thinking on this. response name as a term is also uses in most GraphQL server implementations but there is now note explaining what it is. Lets see if we can combine the terms, both mean the same thing. Response key is more used when talking about the key in the map I think. But we should just use a single term here.

[michaelstaib]

Just checked ... graphql-js as the reference implementation only uses responseName

function executeFieldsSerially(
  exeContext: ExecutionContext,
  parentType: GraphQLObjectType,
  sourceValue: unknown,
  path: Path | undefined,
  fields: Map<string, ReadonlyArray<FieldNode>>,
): PromiseOrValue<ObjMap<unknown>> {
  return promiseReduce(
    fields.entries(),
    (results, [responseName, fieldNodes]) => {
      const fieldPath = addPath(path, responseName, parentType.name);
      const result = executeField(
        exeContext,
        parentType,
        sourceValue,
        fieldNodes,
        fieldPath,
      );
      if (result === undefined) {
        return results;
      }
      if (isPromise(result)) {
        return result.then((resolvedResult) => {
          results[responseName] = resolvedResult;
          return results;
        });
      }
      results[responseName] = result;
      return results;
    },
    Object.create(null),
  );
}

above the serial execution of graphql-js. The term responseKey is not used on graphql-js. Same goes for HotChocolate, which also only uses responseName. I will do another PR getting this fixed in the spec. Will also add a note.

[mjmahone]

Looking at the broader context, parallel here means fields that can be independently computed, as used above, in https://spec.graphql.org/draft/#sec-Executing-Selection-Sets. As in, they may be executed in parallel, as they have no serial dependency.

When more than one field of the same name is executed in parallel, their selection sets are merged together when completing the value in order to continue execution of the sub-selection sets.

This could instead change to: "When more than one field of the same response name may be executed in parallel, their selection sets are merged together when completing the value in order to continue execution of the sub-selection sets."

Additionally, we could maybe add a note: "Note: Fields that must be executed serially, in contrast, may not share the same response name."

But this whole section does not make sense except in the context of section 6.2 and 6.3, which define the difference between serial and parallel execution. I cannot easily come up with a better concept for what we're describing besides parallel field execution.

[rivantsov]

I think there's some confusion here. The original text gives a (wrong) idea that merge/not merge may depend on parallel/serial execution mode chosen by the server - which it free to choose. I asked at the meeting, "The merge/no merge decision does NOT depend on execution order, right? " - just to state it clearly, before we discuss wordings. And we all agreed, YES. Parallel or serial - it is MERGED field in the output It makes sense, if we say that server is free to chose exec mode, we can say it ONLY if output is the same. But the old wording is hinting that merge is only in parallel execution mode. Matt, it looks like you now try to resurface the idea that it DOES depend on execution mode. I think we should make it clear that merge/no merge has nothing to do with parallel/serial. And that was the reason to remove this "in parallel".

[rivantsov]

I understand you try to use 'if can be done in parallel' as criteria for 'should be merged', but I think this brings more confusion. I think parallel should be taken out completely. I do agree that 'parallel fields' in the next sentence should also be taken away

[rivantsov]

I looked at other places where Field merge is discussed and I think there's more wordings to fix there. I am closing this PR to open a discussion first and then a PR.