Improve error code handling in fed 2 code

[pcmanus]

The current handling of error codes in the current federation 2 code is a bit haphazard. In particular, it doesn't consistently include an error code for all composition errors, nor are new codes documented. Plus, quite a few errors don't have any exercising tests.

This PR aims at fixing this situation.

Concretely, this PR makes the choice of grouping the declaration of all error codes in a single file (internals-js/src/error.ts) and to declare them with a few additional metadata: a description, the version in which the code was introduced and, when a code replaces another now removed code, the replaced code(s). This allows the errors.md documentation to now be generated (there is a super-simple script added to do it, result here). Hopefully this will help keeping that part of the documentation more up-to-date with the code/simplify the maintenance burden.

A few notes regarding that (now generated) documentation:

• because it's generated, manual changes to errors.md will now be discarded, and that fact is not very clearly flagged right now (I could maybe add a html comment in the file, would that bother gatsby?). Any strong objections to such generation? @StephenBarlow in particular?
• I've added the generation of said file to the npm run compile target as a last step. It's pretty fast so I assume it's not a issue but not married to that method.
• the generated doc should have everything needed to navigate the change in error codes that fed 2 brings. That is, every 0.x error code should be either:
  1. reused by fed 2. Fed 2 tries to preserve error codes if it make sense.
  2. mentioned as a "replaced" code. This is done when the error is still an error in fed2 but the old code name was too misleading (code starting with VALUE_TYPE for instance where "value type" is not that meaningful a distinction and could even confuse users).
  3. in the list of removed codes: those are listed at the end of the doc with a quick description of why they are not needed/relevant anymore.

[trevor-scheer]

Can we enforce the error doc is updated in CI similar to the above codegen:check?
The related Circle job is here: https://github.com/apollographql/federation/blob/main/.circleci/config.yml#L84-L94

[pcmanus]

Yes, that make sense and I did that. I am a circleci n00b however so appreciate if you can double-check 8e14b8c (not too worried cause I genuinely just copied what codegen:check but ...).

[clenfest]

Not done yet, but I wanted to get the big comment to you as soon as I could.

[clenfest]

Wondering if it might make sense to separate this out into multiple steps since compile is what developers are likely to run and probably don't want to generate docs every time they recompile.

[pcmanus]

Yeah. As I mentioned above, I'm pretty sure the generation here is fast to the point of not making measurable differences to the compile target and that was meant to ensure we don't forge generation.

But Trevor suggested a better approach above, having CI double-check that don't forget generation, so did that and removed it from compile.

[clenfest]

Given that we have things exported in this file that are not errors, I'm wondering if it might be valueable to further namespace the errors. i.e. declare them all as const and export them later with:

export const Errors = {
  ERR_FIELDS_HAS_ARGS_CATEGORY,
  ...,
}

Alternatively we could separate this into two files, where one is just for the errors. Let me know what you think.

[pcmanus]

Good point. Namespacing that way does clean things up a bit, and that makes the added a good enough registry for me (more on that below).

[clenfest]

It looks like a bunch of these exported definitions aren't actually used externally. I think there is a linter rule for this, but in the meantime I'd just get rid of the export on anything that isn't used outside of this file.

[pcmanus]

I kind of disagree here (and I would be against a linter rule that reject this). The error code metadata is used externally, by the doc code generation. Granted, the structural typing of typescript make it so that ErrorCodeMetadata is essentially just a name and external code can still describe the type of metadata "manually" without that name if they need to, but if we feel the need to introduce a meaningful name to more conveniently refer to the metadata type, why would we refuse it to others when metadata are exported (and meant to)?

More generally, while I'm all for being careful with what gets exported, I don't think that "only export what is used externally at the time of the writing" is a good rule. The whole intend of the approach of this ticket is to make it easier for "external" code to reason about which error code exists and have details on them. Yes it's only used by the doc generation right now, but I could easily see studio eventually have a use for this. And if they do, and do want to refer to the type of metadata in they code, then I could easily see them not bothering opening up a PR to export ErrorCodeMetadata then, but instead either redefine it, or inline the type making their code less readable, neither is ideal. To be clear, it's a very minor detail in this case admittedly. I'm just trying to justify why I think it's sometimes ok, and even sometimes a good thing to export thing even if they are not used right away (sometimes, not always, YAGNI is a thing for sure). And I think the exporting is justified here and I'd rather keep it.

[clenfest]

I started making comments in this file which I'll leave but I may duplicate myself here. A couple of principles I use when writing typescript code.

• Prefer types / interfaces to classes - Unless classes are really bringing something to the table in terms of polymorphic inheritence we should prefer just using a type or an interface. If you look at your code below, class is overly heavyweight for ErrorCodeDefinition as each definition really only has 3 properties and 1 function. Similarly FederationDirectiveErrorCodeCategory and ConcreteErrorCodeCategory are really just mechanisms for creating an ErrorCodeDefinition, they don't need their own classes. I would convert ErrorCodeDefinition into a type and then create a couple of different generator functions (makeError, makeConcreteError, makeDirectiveError). I did a bit of a rewrite just to prove to myself everything would work out, and came up with this as a start:

type GraphQLErrorArgs = {
  message: string,
  nodes?: readonly ASTNode[] | ASTNode,
  source?: Source,
  positions?: readonly number[],
  path?: readonly (string | number)[],
  originalError?: Error | null,
  extensions?: { [key: string]: unknown },
};

type ErrorCodeMetadata = {
  addedIn: string,
  replaces?: string[],
};

const FED1_CODE = '0.x';
const DEFAULT_METADATA = { addedIn: '2.0.0' };

type ErrorGeneratorFunc = (args: GraphQLErrorArgs) => GraphQLError;
export type ErrorCodeDefinition = {
  code: string,
  description: string,
  metadata: ErrorCodeMetadata,
  create: ErrorGeneratorFunc,
};

const makeError = (code: string, description: string, metadata: ErrorCodeMetadata = DEFAULT_METADATA): ErrorCodeDefinition => ({
    code,
    description,
    metadata,
    create: ({
      message,
      nodes,
      source,
      positions,
      path,
      originalError,
      extensions,
    }: GraphQLErrorArgs) => new GraphQLError(
      message,
      nodes,
      source,
      positions,
      path,
      originalError,
      {
        ...extensions,
        code,
      },
    ),
  });

export const ERR_TAG_DEFINITION_INVALID = makeError('TAG_DIRECTIVE_DEFINITION_INVALID', 'The @tag directive has an invalid defintion in the schema.', { addedIn: FED1_CODE } );

• Prefer named arguments - My personal opinion is that named arguments are a good idea if you have more than one or two arguments to a function. They protect you from changes in a way that positional arguments don't. See the type I created GraphQLErrorArgs for the create function type declaration above. The downside is that clients will need to specify the name rather than the position, but they will also know immediately if they are invoking incorrectly, which they won't if there are a bunch of optional parameters or arguments of the same type. In the below example, if we wanted to make foo an optional parameter for some reason, we'd need to move it to the end and then change the ordering for each time it was invoked, whereas in the named parameter version it would be very easy.

const positionFunc = (foo: string, bar: string, baz: string) => {...};
positionFunc('a', 'b', 'c');

const namedFunc = (
  { foo, bar, baz }: 
  { foo: string, bar: string, baz: string}
) => {...};
namedFunc({
  foo: 'a',
  bar: 'b',
  baz: 'c',
});

I'm not sure whether or not you need a registry or not. If the only reason to have it is to be able to iterate over errors to generate documentation, I think we can just get rid of it and use the following trick.

1. Don't export anything from error.ts other than actual errors or types (this is why I made ErrorCodeDefinition a type above rather than an interface). If you want to make it an interface or export other things, I'd break it up into two files and ensure that one of them only exports errors.
2. Write a test to ensure that all exported members are in fact ErrorCodeDefinitions
3. To iterate over errors in genErrorCodeDoc.ts, you can do something like the following which will typecheck safely.

import * as importedErrors from './error';
import type { ErrorCodeDefinition } from './error';

// the following casting is safe because we have a test that ensures all members of 
// error.ts are ErrorCodeDefintions
const errors = new Array(importedErrors as unknown) as ErrorCodeDefinition[];
for (const err of errors) {
  console.log(err.code);
}

If you decide you want to keep the registry, have makeError add the ErrorCodeDefinition to the registry before returning and make sure makeError is called by all versions of functions that make an error.

[pcmanus]

Thanks a lot for these remarks. I say this first because I'm going to push back a little bit on some of it, but I don't want it to sound like I resent your remarks in any way. On the contrary, I push back because I suspect you may have a point that I just don't understand and I'd like to learn.

Prefer types / interfaces to classes - Unless classes are really bringing something to the table in terms of polymorphic inheritence we should prefer just using a type or an interface.

I've changed it here because it's true the classes don't add much value and I don't want to sound too contrarian.

But I can't adopt a principle without understanding it well, and at the time of this writing, I'm not yet convinced on that one (but maybe I'm reading your "prefer" above more strictly than you intended?).

I'm definitively not saying "classes are better, let's prefer them everytime we can" but I also don't see why classes would be strongly avoided "except for inheritence". In particular, I feel classes have a bunch of advantages unrelated to inheritence (the later of which I'm all for not abusing btw) that make them nice in a fair number of situation:

• they seem to be better at encapsulating private state (and granted, private offers only so much protection in typescript, but it's imo better than nothing).
• they also lock-up things more than types in the following sense: a type is an invitation for others to provide their own implementation, so when you add method to a type, you're less sure that you're not going to break some custom implementation you're not aware of. If you have a class, you've signaled that people should just use the class (or extend it, which is fine) and you can add methods without worries. That kind of lock-up is not always desirable, for sure, but I routinely run in cases where it's a plus, not a minus.
• they group typing and implementation better, which imo make things more readable at times. With types/interfaces, you just get the type and so you still need an implementation, and especially when you start having many fields and methods, that can duplicate code, on top of physically separating the code from the type it implements. Of course, there is plenty of cases where having a separate type/interface is exactly what you want, but there is imo plenty of other cases when having the type separated doesn't matter and then a class can offer an edge on readability.

So when you say "If you look at your code below, class is overly heavyweight for ErrorCodeDefinition", I ... don't really see it. I'm not sure what "overly heavyweight" refers to (and again, maybe I'm overreacting to your phrasing, but I read "overly heavyweight" as a pretty strong statement)?

More precisely, ignoring other changes like using named parameters (which do improve things), the type and the class versions are not that different in terms of lines of code, nor does one of them looks a ton more readable to me (I get that one can prefer one over the other, and I have my own preferences, but I can't find arguments to justify why one would be objectively much more readable).

And to my points above, with the type version, if I want to add a new method, I have to add it in 2 places instead of just 1 with the class. Not a huge deal certainly but .... And while in exchange we get the value of a type that can be easily implemented externally, it's of unclear value here. Possibly I'm missing something and the type version has a clear edge on maintenance, but I'll admit it's not clear to me.

Or is it maybe mainly about performance? I could imagine classes having a greater performance cost, though I haven't found much data on that (but I'd be very interested by some pointers on this). Event then, surely we're not talking massive differences and especially for error code handling, that feels at best like a small point.

Again, to be clear, I do have been writing way too much Java these last few years and I don't deny that I may jump to classes a bit too quickly and need to reeducate myself. And I'm ok with trying to use types/object literal more often when using classes don't bring much benefit (like in this example), even though I'm still unclear on why the type/object literals is so much better as you seem to suggest.

But I'd definitively need more justification to get behind a rule that says to almost universally avoid classes.

Prefer named arguments

Make sense. And I'll definitively try to follow that moving forward.

Fwiw, part of why I used positional argument here is that 1) I was mimicking the GraphQLError api, and 2) the overwhelming majority of our use cases only pass only 1 or 2 arguments, making positional argument almost nicer in practice in that case. But I do agree named arguments are just better for more than a couple arguments and no point in making an exception here, so changed it.

I'm not sure whether or not you need a registry or not.

I really do want one :). That is, back to my earlier comment, I do think a benefit of the approach in this PR is to make it easy to programmatically reason about all error codes, and having to rely on a trick doesn't make sense to me.

That said, all I'm talking about is a simple way to iterate over all errors and the change you suggested above of namespacing all errors in a Errors const essentially give us that (through Object.values), so I removed the dedicated registry class and it's certainly simpler.

[clenfest]

In ES5, classes were terribly inefficient and also made the debugger sad, but on modern versions of the language it's not an issue anymore, so I probably need to get it out of my head to use them as a last resort. However, I still think in this case that having multiple classes isn't the right choice, primarily because the only thing that's different between the different classes is the constructor, and there are no methods. If you wanted to have a single class, I'd probably be ok with it.

On the bullet points you have above, I agree that when you have multiple methods and specific ways that you want clients to interact with the data, they are a good choice. But if the type is just a constructor and a way to tie data together, I prefer the interface/type approach. Not sure what IDE you use, but it's a lot easier to just hover over a type and be able to see all members of that type. For a class you have to click through.

There is a linter rule that I'll enable at some point called class-methods-use-this which might make it easier for us to choose when to use which. Basically the rule says that if you aren't referencing this in your methods, they should probably be static methods rather than class methods. If all methods are static, then maybe the class shouldn't exist.

PS, thanks for pushing back. I do think this kind of discussion makes us better developers and so I welcome it.

[pcmanus]

I still think in this case that having multiple classes isn't the right choice

Yeah, as I said, I was reacting more to the general idea of classes being only for "last resort". I'm ok with using types here (and I've changed it).

If all methods are static, then maybe the class shouldn't exist.

Right, I'd get behind that :)