reactphp / promise

Promises/A implementation for PHP.
https://reactphp.org/promise/
MIT License
2.38k stars 146 forks source link

[3.x] Add template annotations #188

Closed simPod closed 1 year ago

simPod commented 3 years ago

I was considering adapting this lib for promises though it's currently impossible to be type-safe with it.

I found this psalm plugin https://github.com/Bocmah/psalm-reactphp-promise-plugin that only includes stubs for psalm. So I was thinking it can be simply ported into this promise lib so it's also supported by phpstan and by any static analysis overall without need for any additional setup.

simPod commented 3 years ago

@clue thanks for input. I'll look into how to test this.

Also note this is only partial implementation (for happy path) as both psalm and phpstan do not support default template types yet (it's impossible to resolve template to a type when null is passed).

This is resolving onFulfilled's type but not onRejected's one. I thought it might be a good start anyway to support at least one side for now. Or we keep it here and build up as the support emerges.

Crosslinks:
https://github.com/vimeo/psalm/issues/5407
https://github.com/phpstan/phpstan/issues/4801

On top of this, phpstan also supports generics, does it make sense to address this as part of the PR?

phpstan reads @psalm- annotations so this should be addressed if you meant this.
(personally, I don't use prefixes at all in my code but as I ported the initial implementation from psalm plugin, I kept them)

simPod commented 2 years ago

I'll wait for default promise value support in SA since it's fairly useless without it

SimonFrings commented 2 years ago

@simPod thanks for your work on this so far :+1:

To keep you updated: @WyriHaximus, @clue and I are currently looking into this, we'll have a call later this day to discuss the details. There's also a discussion in pslam with more information on this: #7559.

simPod commented 2 years ago

Thank you for update and pushing it forward!

WyriHaximus commented 2 years ago

One major thing for me was to tackle the bigger picture, so not just this promise package. But also async and await and observables plus await with observables and get it right as a whole.

WyriHaximus commented 2 years ago

@simPod Just a heads up: I'm going to make a bunch of suggested changes based on https://psalm.dev/r/22a9692a97 later today. But want thank you for your work so far on this and for pushing it forward so far :+1:

WyriHaximus commented 2 years ago

FYI just put up #223 as a draft for the 2.x branch so I can pull it in through composer and test it out again in a project without having to manually edit files again. Will try to get back shortly and hope live doesn't get in between again

WyriHaximus commented 2 years ago

FYI just filed https://github.com/reactphp/promise/pull/227 and https://github.com/reactphp/promise/pull/228 those will not have any direct affect on this PR as it's for the upcoming v3 release. We want basic support out there and then work on getting near to a 100% support out there through this PR and PR's for 1.x and 2.x.

WyriHaximus commented 1 year ago

@simPod Hey if you don't mind I'm going to use your commit as a base to get template annotations + some additional type insurance in.

simPod commented 1 year ago

Awesome 👌🏿

On Fri, Jun 23, 2023, 01:00 Cees-Jan Kiewiet @.***> wrote:

@simPod https://github.com/simPod Hey if you don't mind I'm going to use your commit as a base to get template annotations + some additional type insurance in.

— Reply to this email directly, view it on GitHub https://github.com/reactphp/promise/pull/188#issuecomment-1603368759, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACQAJLSS6A4Q46K5ZQJXKLXMS57TANCNFSM4ZGKGHSA . You are receiving this because you were mentioned.Message ID: @.***>

WyriHaximus commented 1 year ago

PR is up at: https://github.com/reactphp/promise/pull/247