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

Implement MrHorse.parallel, switch Async to Items #17

Merged
merged 10 commits into from
Jul 9, 2015

Conversation

devinivy
Copy link
Collaborator

@devinivy devinivy commented Jul 7, 2015

Here's MrHorse.parallel, currently sans documentation. See tests for usage for now. Happy to answer any questions about this! Builds on top of work done in #15. Related discussion: #11.

Short example config from tests,

server.route({
    method : 'GET',
    path   : '/parallel-ok',
    handler: function (request, reply) {

        reply({
            handler: 'handler'
        });
    },
    config : {
        plugins: {
            policies: [ MrHorse.parallel('postHandler', 'postHandlerAnother') ]
        }
    }
});

@devinivy
Copy link
Collaborator Author

devinivy commented Jul 7, 2015

Ah, didn't take into account shrinkwrapping. CI should be happy as of the shrinkwrap update in devinivy@a3158e3. Also memoized the function that determines an apply point from a list of policy names.

@devinivy
Copy link
Collaborator Author

devinivy commented Jul 7, 2015

Added syntactic sugar. Example in the first comment of this PR can be rewritten alternately as,

server.route({
    method : 'GET',
    path   : '/parallel-ok',
    handler: function (request, reply) {

        reply({
            handler: 'handler'
        });
    },
    config : {
        plugins: {
            policies: [
                [ 'postHandler', 'postHandlerAnother' ]
            ]
        }
    }
});

@devinivy
Copy link
Collaborator Author

devinivy commented Jul 7, 2015

Added some docs.

@mark-bradshaw
Copy link
Owner

Thanks Devin. I don't have availability tonight to look over and comment.
I'll take a look tomorrow.

On Tue, Jul 7, 2015 at 2:12 PM devin ivy notifications@github.com wrote:

Added some docs.


Reply to this email directly or view it on GitHub
#17 (comment).


##### Running policies in parallel
If you'd like to run policies in parallel, you can specify a list of loaded policies as an array or as individual arguments to `MrHorse.parallel`.
When policies are run in parallel, expect all policies to complete. If any of the policies specify an error or `Forbidden 403` message, by default the first listed policy with such an error will be given precedence.
Copy link
Owner

Choose a reason for hiding this comment

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

What about changing "by default the first listed policy with such an error will be given precedence." to "the error response from the left most policy will be returned to the browser."

Copy link
Owner

Choose a reason for hiding this comment

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

Also, we need to be clear that parallel only accepts strings, not functions, since the last param can be a callback. Right?

@mark-bradshaw
Copy link
Owner

So I plowed through the PR. Basically, it looks good. I had some comments and questions as I went. I think it might be worth taking another look once the dust settles and see if some refactoring might make things simpler, and I definitely agree that we need to be caching the list of policies to run instead of recalculating on each request. Another thing I'd like to see (another PR) is to split out some of the helper functions into a separate file to cut down on line count.

If you could go through the comments and either answer questions or adjust what seems to make sense I'd appreciate it.

@devinivy
Copy link
Collaborator Author

devinivy commented Jul 9, 2015

Thanks for taking such a detailed look! Let's get this right, so that we can feel good about the next release. I'll try to resolve these line notes tomorrow.

@devinivy
Copy link
Collaborator Author

devinivy commented Jul 9, 2015

I made the suggested changes, and tried to clarify some wording. I also added an example to the docs of using currying with function-policies, since I think that may be a desirable use-case.

@mark-bradshaw
Copy link
Owner

Looks good. Merging.

mark-bradshaw added a commit that referenced this pull request Jul 9, 2015
Implement `MrHorse.parallel`, switch Async to Items
@mark-bradshaw mark-bradshaw merged commit 866ca70 into mark-bradshaw:master Jul 9, 2015
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

2 participants