Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle fields omitted through @skip or @include #3

Closed
wants to merge 2 commits into from

Conversation

CRC-Mismatch
Copy link
Owner

@CRC-Mismatch CRC-Mismatch commented May 30, 2023

spawnia#77

  • Added automated tests
  • Documented for all relevant versions
  • Updated the changelog

Builds upon spawnia#79

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 will have the missing fields set to ObjectLike::UNDEFINED to mark them as such.

Breaking changes
Code that used to rely on exceptions being thrown when some property wasn't present in the response will need rework, as not only the exception will only be thrown if the __typename property isn't present, but also the result object will have the missing properties with the string value for ObjectLike::UNDEFINED.

@CRC-Mismatch
Copy link
Owner Author

CRC-Mismatch commented May 30, 2023

@spawnia I didn't want to pollute the original repo while testing and tweaking, so I used my own as a "test bench" for the PR checks. I did reach a solution, but I'd first like to know if my idea is on the right track in your opinion before proposing it on the original repo. This was a "K.I.S.S." solution (I could also set the fields to null, but that would require annotating them as such, and I do believe my approach would still confuse PHPStan since non-nullable properties aren't also typed as possible strings), otherwise we'd have to keep track of which fields could be omitted on each class upon generation.

@spawnia
Copy link

spawnia commented May 31, 2023

The fields should definitely be marked as nullable when the query has @skip or @include on them. Code generation must take that into account. Just setting any field to the magic UNDEFINED string seems wrong on so many levels - it puts all the burden on the user of the generated code and disregards any kind of type safety whatsoever.

@CRC-Mismatch
Copy link
Owner Author

CRC-Mismatch commented May 31, 2023

That's what I thought 🤣 and the reason I wanted to keep it separate at first...
I did try something out earlier, but went nowhere with that approach, and just had a 💡 moment with this simple/naïve approach. But thinking further since yesterday, this gave me an idea on something that could work. I'll work on it today and discard this PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants