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

Refactor Promise classes with generic types (fast-tracking https://github.com/php-http/promise/pull/24) #27

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Refactor Promise classes with generic types (fast-tracking https://github.com/php-http/promise/pull/24) #27

merged 3 commits into from
Oct 24, 2023

Conversation

Ndiritu
Copy link
Contributor

@Ndiritu Ndiritu commented Oct 23, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #23
Documentation see below
License MIT

What's in this PR?

This PR refactors the Promise interface and concrete classes to use generic types. This allows to hold a meta-reference to the type of value the promise will resolve to. The then method will return a separate template type, so you can actually build properly typed then-chains.

For the Promise template, a covariant template type has been introduced. This allows library authors to create their own, constrained Promise types (say, UserPromise).

All in all, these annotations will improve type safety in a lot of code bases.

Regarding the docs
I'm not quite sure whether these additions should be added to the documentation (probably). WDYT?

Example Usage
I'm copying the (contrived) example from #23 here:

class PromiseFactory
{
    /**
     * @template T
     * @param T $value
     * @return Promise<T>
     */
    public static function make(mixed $value): Promise {
        return new SomePromiseImplementation($value);
    }
}

/** 
 * @return Promise<string>
 */
function foo(): Promise {
    return PromiseFactory::make('test');
}

// Inferred to be a string
$stringValue = foo()->wait();

// Static analysis tools will be able 
// to figure out all types involved here!
$floatValue = foo()->then(fn($value) => parseFloat($value))->wait();

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

To Do

  • Discuss documentation changes

Radiergummi and others added 3 commits October 18, 2023 13:48
This commit refactors the Promise interface and concrete classes to use generic types. This provides better support for static analysis tools, enforcing type safety and improving code readability.
@Ndiritu
Copy link
Contributor Author

Ndiritu commented Oct 23, 2023

fast-tracks #24. No intention to take credit for @Radiergummi's contribution.

@Ndiritu Ndiritu marked this pull request as ready for review October 23, 2023 08:02
@dbu
Copy link
Contributor

dbu commented Oct 24, 2023

thanks a lot @Radiergummi and @Ndiritu !

@dbu dbu merged commit c25665f into php-http:master Oct 24, 2023
@dbu
Copy link
Contributor

dbu commented Oct 24, 2023

https://github.com/php-http/promise/releases/tag/1.2.0

@mpyw
Copy link

mpyw commented Oct 24, 2023

The PHPStan CI in our product was broken by this change. It doesn't matter in real operations since it was only a PHPDoc change, but enough to be BC for a CI/CD pipeline with static analysis.

Personally, I welcome this improvement and do not want to see a revert, but in the future, we should be careful about making changes that narrow the type in minor version upgrades, even if they are within the scope of PHPDoc.


As a similar example, the following changes in Laravel have been reverted as BC: laravel/framework#48562

@dbu
Copy link
Contributor

dbu commented Oct 24, 2023

do you implement the interfaces in your application? or does it trigger a phpstan error even when only using the interface?

as this is a documentation only change, and we made it a new minor version, i think reverting it would be overreacting. i am sure the phpstan warning can either be fixed or added to the baseline if you are in a hurry to release with the project with a new minor version of the promise package. (also, there is literally only this change in the release, so you could just stick to 1.1 until you want to upgrade)

@mpyw
Copy link

mpyw commented Oct 24, 2023

We have the class that extends SlackErrorPlugin and override the handleRequest() method. With the addition of the generics parameter, I had to explicitly write a PHPDoc to pass CI.

I do not agree with reverting the PR this time, of course. But we need to be more careful in the future.

@martinssipenko
Copy link

@mpyw could you share an example of working PHPDoc for the handleRequest method?

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.

Generic annotations
5 participants