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

Proposal: Generic types for DTO casting #396

Open
wants to merge 5 commits into
base: v3
Choose a base branch
from

Conversation

michaeldyrynda
Copy link

@michaeldyrynda michaeldyrynda commented Apr 7, 2024

I've added annotations for the dto and dtoOrFail methods, enabling passing of a class-string to each method, in order to get proper type suppose when working with the response.

The new $dto parameter is optional to enable backwards-compatibility, but means that you can call the method with a FQCN passed as an argument, in order to get type completion.

Omitting the argument continues as currently.

I was inspired by @ash-jc-allen's blog post, which passed the FQCN to this method, which initially looked unnecessary to me.

public function getRepo(string $owner, string $repo): Repo
{
    return $this->client()
        ->send(new GetRepo($owner, $repo))
+       ->dtoOrFail(Repo::class);
}

This implementation conforms to PHPStan requirements, so I believe it to be technically correct, but I wasn't actually able to verify it in my editor (Neovim).

For it to function (for me), I had to tweak the annotation slightly:

  * @template T of object
+ * @param class-string<T>|null $dto
- * @return ($dto of class-string<T> ? T : object)
+ * @return T

@juse-less
Copy link
Contributor

juse-less commented Apr 8, 2024

@michaeldyrynda Have you tried with both Intelephense and Phpactor?

My experience is that Phpactor works far better.

It shouldn't be the reason, but I also realise that the return type of the DTO methods is mixed rather than object.
Have you tried change it to object, as well as your conditional fallback return type to mixed?

In terms of changing this type, static analysis tools will start failing for those of us who use custom responses and override the DTO methods directly.
I'm not sure if that strictly qualifies as a breaking chance, though.

I have actually managed to solve DTOs, by defining them in the request, and have them propagate to the DTO methods in the response.
But PhpStorm (that I primarily tested in) doesn't work with cross-file generics like that.
So I gave up rather than propose it.
PHPStan was satisfied, though, and I could perfectly fine use \PHPStan\dumpType() the correct type from the DTO methods.

I can commit and push it if someone is interested anyway.

@ash-jc-allen
Copy link

Hey all! I was just looking through my blog post and agree that it was unecessary for me to add that parameter. I think it was a typo on my end that slipped through my proof reading haha 🤦

Although, I do like the idea of getting some kind of autocomplete and static analysis in my IDE, so I'd be a fan of this addition 🙂

@michaeldyrynda
Copy link
Author

I have actually managed to solve DTOs, by defining them in the request, and have them propagate to the DTO methods in the response.

Agree; I typically use resource-based SDK builds like the docs suggest, so do my typed returns on the connector, rather than on the request itself.

I've tried with phpactor and it makes no difference sadly (though I'll be making the switch to that permanently regardless, it provides better diagnostics than intelephense!), which is a shame.

Wish there was a better way of handling this, as having to annotate things by hand is grim.

juse-less

This comment was marked as resolved.

@juse-less
Copy link
Contributor

I did some tests with the proposed solution, and it works with Phpactor in Neovim, albeit with some adjustments.
I haven't tried Intelephense, though.

I set up a fresh Neovim using NvChad, and then just installed Phpactor.

Interestingly enough, PhpStorm doesn't handle it at all.
But it's also in-line with my experience of the two - Phpactor being far more PHP-intelligent than PhpStorm.

Screenshots of relevant code in Neovim and PhpStorm

Neovim:
CleanShot 2024-04-12 at 22 41 46@2x

PhpStorm:
CleanShot 2024-04-12 at 22 48 44@2x

@michaeldyrynda
Copy link
Author

Well I'll be a monkey's uncle; I tried this again today and it seems to be working as per the PR.

CleanShot 2024-04-13 at 13 02 16

Unfortunately, making the change as @juse-less suggested raises the ire of PHPStan.

CleanShot 2024-04-13 at 13 06 07

So it would seem that the annotation is correct, and functions with both PHPStan and Intelephense. I believe there's a PHPStan plugin for PhpStorm that should enable intelligence for this?

As it stands, the PR is correct, appeases static analysis, as well as completion for the two intelligence tools. Given that the annotations are PHPStan ones, I think it's reasonable for it to only work in that context.

Whether or not this is considered breaking, however, is another story.

@michaeldyrynda michaeldyrynda marked this pull request as ready for review April 13, 2024 03:45
@juse-less
Copy link
Contributor

Ohh. Right.
I believe it's complaining when passing null (or nothing), because T doesn't exist without the class string.
So the check should be to check if $dto is null, and, if not, then return T.

/** @return ($dto is null ? object : T) */

I've always gotten complaints that the template has to be used on the param.
So interesting that it works.
It's entirely possible it's changed more recently and I just haven't checked.

In terms of PHPStan in PhpStorm, it's bundled and didn't make a difference in my screenshot.
But that plugin seems quirky. For things like class-string, you need the bundled Psalm plugin to be active, f.ex.
I do have both active, but no dice.

If it works as proposed, then I don't really see any issues with it per se.
Just that PhpStorm won't have autocompletion.

@Sammyjo20
Copy link
Member

Hey @michaeldyrynda Thank you so much for this PR, I really like this idea and thank you for contributing too @juse-less ❤️

How do we feel about implementing a type check so if someone accidentally puts a different class in, it throws an error? Or even passing the class as a second argument like:

createDtoFromResponse(Response $response, string $classString)

The latter could open up cool DTO casting like match statements based on the class string 🤔 What do you think? I'm happy to keep it simple though

Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

Awesome PR, thanks @michaeldyrynda!

@Sammyjo20
Copy link
Member

Hmm this is really strange, the DTO type checking isn't working on my local machine (Running the latest PHPStorm 2024.1.2) was this actually still in discussion?

@juse-less
Copy link
Contributor

@Sammyjo20 I wouldn't say it's a discussion.
It's a limitation in PhpStorm itself.

Adding this shouldn't break anything in PhpStorm, from I've seen.
But will add autocompletion for other editors and IDEs.
Once PhpStorm adds the support, it should start working there as well.

I'm not sure there's any tricks that can be done to make it work in PhpStorm.

I can check the things I tested a while back, to see how it works in PhpStorm and Neovim.
But in its current state, it'd have to be merged to the next major.

@Sammyjo20
Copy link
Member

Thanks for the information @juse-less that's certainly a shame as it's the IDE I love to use. I'm still willing to release this PR as it is as it will help PHPStan analysis and as you mentioned, other editors.

@juse-less
Copy link
Contributor

I switch between PhpStorm and Neovim quite a lot, depending on what I do, and where.
I see incompatibilities like these, almost always in PhpStorm.
It's indeed a shame. 😕

The big upside of Michaels proposed solution is that it's so simple.
And it doesn't need much code changes. Neither in Saloon, nor SDKs.

The things I tried for v2, when we added more types and generics, got quite big and cumbersome to use in both Saloon itself, but also SDKs.
The biggest problem being propagating the type/generic across files in a way that all editors and tools understand.

@juse-less
Copy link
Contributor

juse-less commented Jun 9, 2024

@michaeldyrynda Can you just double-check that the changes proposed in the PR works?

The reason I'm asking is because I tested them just now (your latest commit, without the style changes, from the diff view).
It doesn't work for me in neither Intelephense, nor Phpactor.

Changes tested, and screenshot
/**
 * Cast the response to a DTO.
 * 
 * @template T of object
 *
 * @return ($dto is class-string<T> ? T : object)
 */
public function dto(?string $dto = null): mixed

As seen below, it won't suggest getMyProperty() method, nor the myProperty property.
Not even typing out 'my' works for me.


Intelephense:

CleanShot 2024-06-09 at 20 18 40@2x

CleanShot 2024-06-09 at 20 18 32@2x


Phpactor:

CleanShot 2024-06-09 at 20 29 09@2x

CleanShot 2024-06-09 at 20 30 19@2x

However, when I use my amended proposed changes, both of them works.
As in; I get autocompletion.

Changes tested, and screenshot
/**
 * Convert the response into a DTO or throw a LogicException if the response failed
 * 
 * @template T of object
 *
 * @param class-string<T>|null $dto
 *
 * @return ($dto is null ? object : T)
 */
public function dto(?string $dto = null): mixed

As seen below, it suggests the getMyProperty() method, and the myProperty property.
And only them, even without typing out 'my'.


Intelephense:

CleanShot 2024-06-09 at 20 15 52@2x


Phpactor:

CleanShot 2024-06-09 at 20 31 36@2x

I've tested with a fresh NvChad installation, as well as my own config.
Both in Neovim 0.10.

It still doesn't work for me in PhpStorm, though.

In terms of PHPStan, I don't get any errors related to it.
I applied my changes on the latest v3, and ran PHPStan on it.

Screenshot of PHPStan

CleanShot 2024-06-10 at 00 49 56@2x

@Sammyjo20
Copy link
Member

Hey @michaeldyrynda love this PR, any chance you could take a look at this comments and see if you can fix the IDE issues Juse raised?

@Gummibeer
Copy link
Contributor

Just an idea - shouldn't it be possible to get the return type of the createDtoFromResponse method and assign that to the template to get rid of the manual return type definition and just have it working with all currently existing implementations? 🤔
It's more work as the generic has to be passed around several classes. But as it's available as the return already it should be possible.

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.

5 participants