2.x returned promise types #139

Activity

1. clue

   added

   question

   on Apr 25, 2019
2. clue commented on Apr 25, 2019

   

   clue

   on Apr 25, 2019

   Member

   More actions

   @downsider Thanks for reporting, I understand where you're coming from. This looks like a potential duplicate, have you seen #137?

   I agree that the current situation is not exactly ideal in the 2.x version, that's why we've fixed this issue for the upcoming version 3. I don't see anything that we could change about this in the 2.x version without introducing a BC break.

   While this is not exactly ideal in the current version, it's not really a type error in this library. It just means that consumer of this package can not infer the return type of the then() method (and family) is an ExtendedPromiseInterface. Practically, any implementation of these interfaces inside this package is always guaranteed to return this type.

   This isn't really an issue with dynamic typed languages such as PHP, as it will work out its type at runtime. This is however something that static analysis tools will report (for good reasons). This can usually be avoided by adding either additional annotations for your static analysis tools or (what I prefer) by adding assertions to your code like this:

   function foo(): ExtendedPromiseInterface
   {
       // …
   }
   
   $promise = foo();
   
   $promiseChild = $promise->then(function ($result) {
       var_dump($result);
   });
   
   // if promise is an ExtendedPromiseInterface, then promiseChild should be too
   assert($promiseChild instanceof ExtendedPromiseInterface);
   $promiseChild->done();

   This is the long form of the type-unsafe version:

   $promise = foo();
   
   $promise->then(function ($result) {
       var_dump($result);
   })->done();

   I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can reopen this 👍
3. clue

   closed this as completedon Apr 25, 2019
4. downsider commented on Apr 26, 2019

   

   downsider

   on Apr 26, 2019

   Author

   More actions

   Ah, I had only searched issues rather than MRs, although the MR itself is not quite what I'm proposing so I'm hesitant to call this a duplicate.

   Indeed, this does only affect static analysis tools but if you use @return static then all behaviour is preserved, is it not? I'm curious as to why you consider this to be breaking. In what scenario would a break happen?

   My understanding is that this change would be updating documentation; from a code perspective, nothing is going to break because we don't enforce the return type. From a static analysis perspective, all that will have changed is that the documentation will be reporting the existing functionality, which would make this a bugfix rather than a breaking change
5. clue commented on Apr 26, 2019

   

   clue

   on Apr 26, 2019

   Member

   More actions

   The @return static essentially boils down to this: ExtendedPromiseInterface::then() must return an ExtendedPromiseInterface, whereas it is currently technically allowed to return the base PromiseInterface.

   Practically, I don't think this would have a large effect, but it's still a subtle BC break anyway. This package has some 30+ Mio downloads (https://packagist.org/packages/react/promise/stats) and plenty of projects depending on it (http://packanalyst.com/class?q=React%5CPromise%5CExtendedPromiseInterface), so it's not a risk I would feel comfortable with.