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

Replace the queue() function with a static Queue class #82

Merged
merged 1 commit into from
Jan 24, 2017
Merged

Conversation

jsor
Copy link
Member

@jsor jsor commented Jan 10, 2017

We allow swapping out the queue implementation and this must be done in a bootstrap file before executing anything from react/promise.

Consider the case, were a user defines a bootstrap file in the composer.json were the queue implementation is changed.

{
    "autoload": {
        "files": ["src/bootstrap.php"]
    }
}

The bootstrap.php contains the code for swapping out the queue implementation.

queue(MyQueueImplementation());

Depending on the order in vendor/composer/autoload_files.php, our functions.php might be loaded after the bootstrap.php thus making the queue() function unavailable to the bootstrap file.

This patch replaces the queue() function by a static class which is autoloadable.

The only difference is in the API, the behavior is unchanged (note, that this feature is not released yet).

Enqueuing a task:

// Old
queue()->enqueue($task);

// New
Queue::enqueue($task)

Swapping out the queue implementation:

// Old
queue(MyQueueImplementation());

// New
Queue::setDriver(MyQueueImplementation());

This makes the Queue class autoloadable and fixes cases
were a consumer wants to set a queue driver via a bootstrap
file loaded from the autoload/files section of the composer.json.
The bootstrap file might be loaded before the functions.php and
the queue() function is then not available.
@jsor jsor requested a review from clue January 10, 2017 19:11
@jsor jsor added this to the v3.0 milestone Jan 10, 2017
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole feature is currently undocumented, so I'm not sure how either the existing version or the updated version is preferable from a consumer perspective?

What do you think? Should we document this in our README or is this merely internal?

@jsor
Copy link
Member Author

jsor commented Jan 16, 2017

This whole feature is currently undocumented, so I'm not sure how either the existing version or the updated version is preferable from a consumer perspective?

I'm not sure what you mean here. The only difference is the API, behavior is unchanged. I've update the PR description, probably its clearer now what changed.

What do you think? Should we document this in our README or is this merely internal?

Good point, at the moment it is internal. The overall behavior is unchanged to v2. The original motivation for adding a queue has been to reduce recursion and avoid blowing up the call stack (#22).

A side effect is that you can swap out the queue implementation to make react/promise guarantee future-turn resolution (#4), eg. required by the promises/a+ spec.

But this is currently undocumented and not tested.

@jsor
Copy link
Member Author

jsor commented Jan 23, 2017

Ping @clue

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, this is an unreleased feature that is both undocumented and untested, so… ¯\_(ツ)_/¯

@jsor
Copy link
Member Author

jsor commented Jan 24, 2017

@clue Just to be clear, this feature is tested.

A side effect is that you can swap out the queue implementation to make react/promise guarantee future-turn resolution (#4), eg. required by the promises/a+ spec.

But this is currently undocumented and not tested.

This is untested, not documented and currently considered an internal feature.

I'm going to make this clearer by marking everything related @internal in a follow-up PR.

@jsor jsor merged commit 3fd6727 into master Jan 24, 2017
@jsor jsor deleted the queue-class branch January 24, 2017 10:37
@jsor jsor mentioned this pull request Jan 25, 2017
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.

None yet

3 participants