fix(graphqlsp): Bail out of unrollFragment infinite loop
#347
Conversation
|
| if (found === element) { | ||
| // TODO: Instead of bailing out here, we should be able to issue a diagnostic here | ||
| // that tells the user that resolution of a fragment has failed. | ||
| return fragments; |
There was a problem hiding this comment.
Let's add a throws argument or something so we can throw if need be from the CLI
There was a problem hiding this comment.
re. notes in PR description, I don't think this is really safe to merge, but instead more for "notes" so I can come back to it 😄
There's several cases where I think similar issues can happen, and there's not really a great option to addressing most of them right now, until we can escalate granular errors, otherwise we basically end up either hiding the issue from the user and it'll only become apparent when they check output (or not at all) or we'd be taking debug info away from ourselves 🤔
NOTE: This is opened as a draft, since it's not a proper fix, but just demonstrates the issue.
Optimally, a proper fix should either:
getDefinitionAtPositiondiagnostics,getDefinitionAtPositionin places where an alternate solution is more reliableIn specific places we should have diagnostics for these cases, since they're usually user errors (or critical errors we can't recover from).
For example, in the changed section in
unrollFragment, we should tell the user that the fragment import is unresolveable.There's also sections for whether a fragment is actually a GraphQL fragment call. If not, we should maybe consider adding a diagnostic to that before bailing out too.