async-interop / event-loop

An event loop interface for interoperability in PHP.
MIT License
170 stars 9 forks source link

Consider using splat operator for $data parameters #79

Closed joshdifabio closed 8 years ago

joshdifabio commented 8 years ago

First of all, if it's been categorically decided that PHP 5.5 should be supported by these specs then feel free to close this issue, since I'm proposing a change which makes use of a PHP 5.6 feature. However, considering 5.5 will be EOL in one month's time, I'm not convinced it should be supported by a spec which will most likely be ratified after that date.

If people are willing to consider bumping the minimum PHP version to 5.6, I propose that the $data = null param be replaced with ...$data in all methods within Loop which use the $data param, and that the splat operator also be used to extract the provided $data when invoking callbacks.

Consider the following (admittedly fairly weird) examples:

Possible approach with current interface

Loop::defer(function (string $watcherId, array $data) { // no type safety for contents of $data
    $data['repository']->save($data['user']); // easy to make a typo when typing array keys
}, ['repository' => $repository, 'user' => $user]); // have to create an array here which is suboptimal

Possible approach with proposed interface

Loop::defer(function (string $watcherId, Repository $repo, User $user) { // type safety
    $repo->save($user);
}, $repository, $user);

I realise the use case here is fairly contrived and doesn't make loads of sense, but hopefully it demonstrates the benefits of using splat over a single $data param.

joshdifabio commented 8 years ago

If people decide they're willing to bump the minimum PHP version to 5.6 then I'm happy to make a PR for this.

trowski commented 8 years ago

Conceptually I like this idea, but I'm concerned about performance. Packing and unpacking an array on every invocation of a watcher method and callback is expensive.

Rarely have I needed to provide more than one value to a callback, as that value is usually an object. I'd rather not slow down the loop for the exceptional cases.

kelunik commented 8 years ago

I had a similar suggestion for Amp's promises. @rdlowrey and @bwoebi were against it. Don't know the reasons anymore.

rdlowrey commented 8 years ago

My hesitation about this isn't really about performance ... it's about allowing people to do things they probably shouldn't be doing.

We all know that fewer function parameters is a better outcome. Monoids objectively require the least cognitive overhead. Two parameters is okay and three is the maximum a good API would ever allow. Just because we can doesn't mean we should. The splat operator gives people license to write excessively complex APIs with many parameters. I personally believe there's real value in providing a well-defined, specific API that requires exactly what applications need at minimum. We should be limiting cognitive overhead in standards -- not trying to accommodate every possible use-case.

My standpoint is that standards should be opinionated; they should accommodate the broadest specific use case. It's my view that something like a splat operator has no business in a standard.

joshdifabio commented 8 years ago

Thanks for the feedback. I don't think this idea is worth pursuing; I'll close this issue soon if no one objects.