Hints improvements #1683
Hints improvements #1683
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
I'll note that the PR uses a generated documentation for hints, like we have for errors, but it's currently a different document. I've be thinking that it might be cleaner to merge the 2 documents together however, so I might try to get a commit for that soon. But I don't think this should hold back review. |
| toString(): string { | ||
| return `${this.code}: ${this.description}`; | ||
| } | ||
| export type HintCodeDefinition = { |
There was a problem hiding this comment.
I think that you'll find that we want to be able to get the english readable level from the HintCodeDefinition. I'd probably go with something like this, though it would involve changing the HintCodeDefinition back to a class and using a constructor
const levels = {
fatal: 60,
error: 50,
warn: 40,
info: 30,
debug: 20,
trace: 10
};
type HintLevel = keyof typeof levels;
export class HintCodeDefinition {
levelNum: (level: HintLevel) => number = (level) => levels[level];
constructor(readonly code: string, readonly level: HintLevel, readonly description: string) {}
}
const INCONSISTENT_BUT_COMPATIBLE_FIELD_TYPE = new HintCodeDefinition(
'INCONSISTENT_BUT_COMPATIBLE_FIELD_TYPE',
'info',
'Indicates that a field does not have the exact same types in all subgraphs, but that the types are "compatible"'
+ ' (2 types are compatible if one is a non-nullable version of the other, a list version, a subtype, or a'
+ ' combination of the former).',
);There was a problem hiding this comment.
english readable level from the HintCodeDefinition
Isn't it what HintLevel[def.code] does? I'm probably brain-farting this but I don't see which advantage the solution above brings (not that I'm against classes :D)?
There was a problem hiding this comment.
Sorry, I'm not seeing how you go from the value to the name (and specifically how rover will do it). If there is a way that I'm not seeing, feel free to ignore.
There was a problem hiding this comment.
Oh, I see what you mean. Forgot we were "blindly" serializing JS object to json for rover.
I'll deal with that. But thinking of maybe have level be a { value: HintLevel, name: string }. Kind of made sense to me to talk of the level as being fundamentally an integer value, but some human-readable name attached for convenience.
There was a problem hiding this comment.
Pushed a change for this. Kept the enum cause it kind of feel appropriate for this to me.
Side-note: I noticed your snippet above included more levels than I have in the patch. I do prefer avoid to add too many levels until it genuinely adds value, but there is of course plenty of space to add more later if the need does arise.
There was a problem hiding this comment.
On the extra levels thing, I copied some code from pino, wasn't suggesting that we actually add all those levels now.
| export type HintCodeDefinition = { | ||
| code: string, | ||
| level: HintLevel, | ||
| description: string, |
There was a problem hiding this comment.
We probably should consider that at some point in the future we may want to merge errors and hints into a single class as I think errors are really just hints that have a higher alerting level. Not sure if there's a case for keeping them separate.
There was a problem hiding this comment.
I generally agree, though there is a bunch of technical considerations to take into account when we do so (probably hard to unplug errors from GraphQLError, but also unclear using GraphQLError for debug level message is exactly "clean"; also, errors are use somewhat more pervasively than hints: maybe that gets in the way, maybe not, but worth looking). So I would prefer leaving any of that to future us for now.
As I mentioned, I do would want to try to at least get the doc merged, so "externally" it looks like those a different levels of the same thing, but I'm gonna call for this to be a followup too at this point.
This commits refactors the hints declaration code to: 1. be consistent with how we declare errors 2. make it easy to generate a doc from it (like we do for errors) It also adds a simple executable to generate an hints doc, which again mirrors what is done for errors. Note that the generated hint document is included in this commit but that nothing is done to have it included/linked by other parts of the doc.
Which outside of wasting cpu, could also have the visible effect of issuing the same hint twice.
This is currently completely unused and and it's unclear there is a good for it short term (the hints already can link to AST nodes, which is location enough in practice). It's always possible to re-introduce this later if we need it but it avoids carrying on dead code in the meantime.
Mechanical renaming meant to further consistency with how errors are defined.
The rational being to provide consistency with error codes.
- renames hints `INCONSISTENT_FIELD_TYPE`/`INCONSISTENT_ARGUMENT_TYPE` to `INCONSISTENT_BUT_COMPATIBLE_FIELD_TYPE`/`INCONSISTENT_BUT_COMPATIBLE_ARGUMENT_TYPE`. The rational is that we have errors with codes `FIELD_TYPE_MISMATCH`/`ARGUMENT_TYPE_MISMATCH`, which would be easy to mixup with the prior hint names. Figured a good way to avoid confusing was have the hint names carry the notion that while it was pointing to an inconsistency, it was a compatible one, but didn't found a better way to do that than to just add `_BUT_COMPATIBLE` to the names. - renames `INCONSISTENT_ENUM_VALUE` to `ENUM_VALUE_MISMATCH`. Did that because it's an error, and all other errors use `MISMATCH` rather than inconsistent (while hints do the reverse) so this is more ... well ... consistent. - switch naming from "execution directives" to "executable directives", because the later is what the graphQL spec uses.
Co-authored-by: weatherman <benweatherman@gmail.com>
pcmanus
force-pushed
the
hints-improvements
branch
from
a451165
to
0f928f2
Compare
That PR contains a number of improvements for hints, but the main goals are to:
WARN,INFOorDEBUG).It also include a number of hints related minor fixes, including those pointed in #1659, and a few rewording of messages (admittedly, many messages could be improved further, but I think this can done post-GA).