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

Resolve ternary types properly #341

Merged
merged 4 commits into from
Mar 23, 2024

Conversation

kburton-dev
Copy link
Contributor

I noticed that some ternary types in my JSON resources were not being resolved properly. For example, a resource could look like this

public function toArray(): array
{
    return [
        'foo' => $this->foo ? FooResource::make($this->foo) : null
    ];
}

Would get a property type such as foo: { type: string }, losing a lot of information about the nested resource.

I hacked together a quick fix for this. I hope I did so in a suitable fashion.

@WildEgo
Copy link
Contributor

WildEgo commented Mar 16, 2024

Could you check how it generates $this->when? I'm not sure how scramble handles this

@kburton-dev
Copy link
Contributor Author

Could you check how it generates $this->when? I'm not sure how scramble handles this

It works fine using ->when()

But then I need to pass a function fn () => null as the third argument. With only null (the default) the property is omitted in the response.

Using a ternary is pretty common, so I think it should be handled well.

Copy link
Member

@romalytvynenko romalytvynenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Small fix is needed to avoid duplicated sub-types in resulting Union and I will happily merge it.

Thanks.

UPD: Also, please update the test to test only Scope class. The only thing that we want to test here is that ternary expression results in Union type when inferring the type, this is not related to API resources.

src/Infer/Scope/Scope.php Outdated Show resolved Hide resolved
tests/TypeToSchemaTransformerTest.php Outdated Show resolved Hide resolved
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.

3 participants