search

spawnia / sailor

A typesafe GraphQL client for PHP
MIT License
78 stars 18 forks source link

Handle fields omitted through `@skip` or `@include` #79

Open spawnia opened 1 year ago

spawnia commented 1 year ago

https://github.com/spawnia/sailor/issues/77

Changes

When an operation tells the server to omit a field through @skip or @include, the result will not contain those fields at all. Thus, the generated result must be marked nullable and be able to handle if those keys are missing.

Breaking changes

I foresee none.

CRC-Mismatch commented 1 year ago

I was checking #77, #78 and this for an upcoming task I'm about to take on later this week or the next.

Is it currently already possible to properly use the directives, or are there still any limitations that this PR is meant to solve?

TL;DR - We need to suppress unneeded queries and results to reduce the performance penalty We're transitioning back-ends from an close-to-unmaintainable REST-only system to a new back-end designed from scratch with GraphQL-only support in mind. To enable the front-end developers to "chop" the transitioning so we can deliver a production-ready migration without having to delay everything for months (or years), we've developed a BFF that "translates" the most coupled legacy REST contracts and endpoints to the new model using Sailor. I've developed a Symfony bundle wrapper ([mismatch/sailor-bundle](https://github.com/CRC-Mismatch/spawnia-sailor-bundle)) to ease its integration in our current projects' architecture. Problem is, we can't deliver the BFF (and by association the clients' migration) as "production-ready" until we can solve the huge performance penalty due to making one REST request become ~15 GraphQL queries and mutations (most of the queries are already compressed in 3 requests, but the mutations are mostly one-per-request) since when we originally started, we couldn't leverage the `@include` and `@skip` directives. Without them, we'll have to create one variation of each query or mutation for each possible scenario where we don't need something, which could solve the performance problem with some clever coding to decide when to use what; but that would mean having around 4x the number of generated classes, and the same amount of new code-paths (incurring in huge NPath and Cyclomatic complexities, in a code that already suffers from these).

I'd like to know the current status, as when I take on that task, if something is still amiss, I'm probably going to first try and work on this PR, instead of turning our BFF's code into a Hydra right away.

spawnia commented 1 year ago

Is it currently already possible to properly use the directives, or are there still any limitations that this PR is meant to solve?

No, the problems with them are detailed in the description of this pull request. Sailor will currently not handle a field missing due to @skip or @include gracefully. Feel free to fork and propose a solution to this pull request, it is currently only a failing test.