Skip to content

Conversation

@nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Aug 6, 2021

This pull request attempts to show what were the pros and cons if we were to introduce serious strong type definitions into Laravel's core. Now, so people can have an idea of what these represent in terms of work, this pull request only includes:

  • Partially migrates the Enumerable interface.
  • Test Enumerable interface type definitions.
  • Adds CI job that to run type definitions tests.

In terms of advantages, we would of course offer much better native support for Psalm and PHPStan. And also, we would have better native auto-completion in editors like PHPStorm:

Before:
before

After:
after

In addition, functions like tap, collect, retry, and more, would 100% static analyzable and understood by static analysis tools.

Now, in terms of disadvantages, it seems that PHPStorm support for generics is very primitive, so there are a few cases where type inference does not work as expected - like VSCode and TypeScript:

$users->each(function ($user) {
    $user->; // $user is mixed...
});

Another disadvantage is the fact that not everyone is comfortable with this type of annotations. So doing it right - for new/existing contributors - may be a friction point.

Finally, the goal of this pull request is not to have this pull request merged, but rather debate if we plan to have this improved type system across the entire code base or not.

@nunomaduro nunomaduro marked this pull request as ready for review August 6, 2021 13:04
@deleugpn
Copy link
Contributor

deleugpn commented Aug 6, 2021

agreed that these stuff are very confusing and add friction. On the other hand if Laravel initiate this movement, it could cascade to a lot of community contribution in educating what are these things and how we use them.

@SjorsO
Copy link
Contributor

SjorsO commented Aug 6, 2021

With these type definitions, does PHPStorm know that User::all()->get(0) returns a User? Or does it only know that it's a Model?

@nunomaduro
Copy link
Member Author

nunomaduro commented Aug 6, 2021

@SjorsO With this changes, PHPStorm will infer that User::all()->get(0) returns a User. Or that a collect([new User])->first() returns a user:

Screenshot 2021-08-06 at 15 23 27

@FrontEndCoffee
Copy link
Contributor

agreed that these stuff are very confusing and add friction. On the other hand if Laravel initiate this movement, it could cascade to a lot of community contribution in educating what are these things and how we use them.

The ideal situation is that it doesn't get in the way of new users, but it does help with users that want this. My team relies heavily on static analysis and typesafety, so this would be an awesome addition.

I do agree that it shouldn't add too much friction for new users, but I don't think it's the case with this implementation.

@deleugpn
Copy link
Contributor

deleugpn commented Aug 6, 2021

The friction is not for users, but for contributors. Laravel gets a great stream of pull requests from people that would all need to learn how to use these stuff. I don't think there's any friction for users?

@ItaloBC
Copy link

ItaloBC commented Aug 9, 2021

The friction is not for users, but for contributors. Laravel gets a great stream of pull requests from people that would all need to learn how to use these stuff. I don't think there's any friction for users?

Not for users, but as long StyleCI points out places where Generics and static types should be pointed, checks would come automatically. Contributors will get a big red X saying why the PR hasn't passes: fix you shit.

Now, I'm not keen to add type definitions on Laravel 8.x. It should be done and mandatory for Laravel 9.x.

@GrahamCampbell
Copy link
Collaborator

StyleCI is not a static analyzer. It's a fully automated code style fixer. It cannot "point out places that are wrong for a human to fix". StyleCI can only detect things it can fix, and then it goes and fixes them on PR merge.

@taylorotwell
Copy link
Member

I do personally think it is in our user's best interest to provide the best type information we can in these situations - so I'm 👍 on improving the DX here.

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.

9 participants