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

Require sync "when" invocations after resolution #26

Merged
merged 2 commits into from
Dec 25, 2016
Merged

Conversation

kelunik
Copy link
Member

@kelunik kelunik commented Dec 23, 2016

It specifies the exact invocation parameter values now and includes a statement about always-sync when callbacks.

Closes #20.

It specifies the exact invocation parameter values now and includes a statement about always-sync when callbacks.

Closes #20.
@joshdifabio
Copy link
Contributor

Does this mean the following needs to be removed from the spec?

All registered callbacks MUST be executed in the order they were registered.

I'm thinking about what should happen if a promise has just been resolved and is in the process of firing it's callbacks (or has deferred doing so) when when() is called.

bwoebi
bwoebi previously approved these changes Dec 23, 2016
WyriHaximus
WyriHaximus previously approved these changes Dec 23, 2016
@bwoebi
Copy link
Member

bwoebi commented Dec 23, 2016

@joshdifabio no, that should still take priority.

EDIT: looks like I was misunderstanding you…

@joshdifabio
Copy link
Contributor

Even though amp doesn't currently satisfy this requirement?

All registered callbacks MUST be executed in the order they were registered.

Is it clear what I mean?

@joshdifabio
Copy link
Contributor

Here's a code example.

function ($promise) {
    $promise->when(function () use ($promise) {
        echo '1';
        $promise->when(function () {
            echo '3';
        });
    });

    $promise->when(function () {
        echo '2';
    });

    $promise->resolve('foo');
}

I think this would print 132 in Amp, even though according to the spec it should print 123? Always-sync when() on resolved promises makes it quite messy to avoid this.

@kelunik
Copy link
Member Author

kelunik commented Dec 23, 2016

@joshdifabio Amp will soon be fixed, I wasn't aware of that.

@kelunik
Copy link
Member Author

kelunik commented Dec 23, 2016

On the other hand, we might just drop that requirement, dunno whether it's important.

@joshdifabio
Copy link
Contributor

I think the existing requirement regarding the invocation order of callbacks is much more important than the one added in this PR, and while it's probably not that hard to support both requirements in Amp it would be extremely difficult for libs with task queues which are deferring invocation to do so.

@kelunik
Copy link
Member Author

kelunik commented Dec 23, 2016

@joshdifabio If we merge this PR, then there MUST NOT be any task queues. This PR exist to eliminate them.

@joshdifabio
Copy link
Contributor

@joshdifabio If we merge this PR, then there MUST NOT be any task queues. This PR exist to eliminate them.

Aha! I hadn't realised.

@kelunik kelunik changed the title Clarify Promise::when docblock Require sync when invocations after resolution Dec 23, 2016
@kelunik
Copy link
Member Author

kelunik commented Dec 23, 2016

I just made the title of this PR clear. It does way more than it actually should. The clarification should be on a separate PR.

trowski
trowski previously approved these changes Dec 23, 2016
@kelunik
Copy link
Member Author

kelunik commented Dec 23, 2016

@jsor Are you fine with that? 10 hours ago you said you'd prefer unspecified, but then you said a always sync implementation is possible in React.

@kelunik kelunik changed the title Require sync when invocations after resolution Require sync "when" invocations after resolution Dec 23, 2016
@joshdifabio
Copy link
Contributor

@joshdifabio If we merge this PR, then there MUST NOT be any task queues. This PR exist to eliminate them.

I just want to clarify; are you saying that React and others would have to completely remove their task queues? (I believe that they would, unless we stop requiring that callbacks are fired in a particular order.)

@kelunik
Copy link
Member Author

kelunik commented Dec 23, 2016

I just want to clarify; are you saying that React and others would have to completely remove their task queues?

They can do what they want. Just not for when. They can continue with task queues for then.

@bwoebi
Copy link
Member

bwoebi commented Dec 23, 2016

I'm not sure, but due to this being a little ambiguous in the text, could you please just specify a bit more precisely when what is called (immediate vs. queue).

@joshdifabio
Copy link
Contributor

As per amphp/amp#57, the following two requirements are mutually exclusive:

  1. If the promise is already resolved, the callback MUST be executed immediately.
  2. All registered callbacks MUST be executed in the order they were registered.

So this PR needs to also remove or modify the second clause.

Callbacks can't be executed always-sync and in the same order as registered in the edge case where another when handler is registered within a when handler. Once a promise is resolved, all new when handlers must be executed immediately now.
@kelunik
Copy link
Member Author

kelunik commented Dec 23, 2016

I just adjusted the PR.

@kelunik kelunik dismissed stale reviews from WyriHaximus and trowski December 23, 2016 22:56

PR changed.

@bwoebi
Copy link
Member

bwoebi commented Dec 23, 2016

Now it's unambiguous :-)

@joshdifabio
Copy link
Contributor

Looks good. Thanks @kelunik!

@kelunik
Copy link
Member Author

kelunik commented Dec 24, 2016

I'd like to await @jsor's approval for this PR.

@jsor
Copy link

jsor commented Dec 25, 2016

@kelunik :shipit:

@bwoebi bwoebi merged commit 614ae88 into master Dec 25, 2016
@kelunik kelunik deleted the when-invocation branch January 20, 2017 15:13
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.

6 participants