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

Support for global view context #6

Merged
merged 3 commits into from Oct 31, 2014
Merged

Conversation

@jagoda
Copy link
Contributor

jagoda commented Oct 30, 2014

Closes #4

@@ -262,6 +264,11 @@ internals.Manager.prototype.render = function (filename, context, options, callb
context = context || {};
options = options || {};

if (typeof this._context === 'object') {

This comment has been minimized.

Copy link
@arb

arb Oct 30, 2014

Contributor

What happens if this._context is a function?

This comment has been minimized.

Copy link
@jagoda

jagoda Oct 30, 2014

Author Contributor

Not supported in this form. I wasn't able to find a good way to make this work with the examples using the request argument.

@@ -454,6 +461,11 @@ internals.Manager.prototype.response = function (template, context, options, req

Joi.assert(options, internals.schema.viewOverride);

if (typeof this._context === 'function') {

This comment has been minimized.

Copy link
@arb

arb Oct 30, 2014

Contributor

What if this._context is an object?

This comment has been minimized.

Copy link
@jagoda

jagoda Oct 30, 2014

Author Contributor

In the current form, I believe that the generateResponse() call below will eventually call render() via internals.marshall and the logic in render() will process the global context.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 30, 2014

I think maybe if they pass a function for the global context, it shouldn't take any parameters, that way it isn't limited to where it's used. If they need something from the request, they'll have it "on hand" when they call reply.view right?

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Oct 30, 2014

Agreed, that will simplify things a bit. Will update accordingly. Thanks for the feedback!

@jagoda jagoda force-pushed the jagoda:feat-global-context branch from 579c9af to c633aa7 Oct 30, 2014
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Oct 30, 2014

Better?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 30, 2014

So I noticed your tests pass query in the context, can you also add a test showing what happens if you've got a query in the path as well as in the global context? Which one can the user expect to be the "winner" in that case?

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Oct 30, 2014

Sure. If I understand correctly, that only applies to the Vision.handler case, right?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 30, 2014

Anywhere were you have access to like the query string or route params. I think that's probably only in Vision.handler, but verify.

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Oct 30, 2014

Vision.handler does indeed seem to be the only place where request specific artifacts are available. I have added test cases for the relevant cases. The order of precedence (highest to lowest) is:

  1. custom handler context
  2. route parameters (default handler context)
  3. global context

This seems consistent with what I saw in the existing code/tests.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 30, 2014

@jagoda somewhere where you do server.inject pass something in the query string. We are trying to test https://github.com/hapijs/vision/blob/master/lib/index.js#L433-L446 to prove what you are saying. We want to engage the hapi code that creates request.query and make sure that it overrides the potential collision with the global context query object.

You'll want to inject a request with a query string and not pass a query as a context value. Make sense?

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Oct 30, 2014

Yup, understood. I believe I did that here. Sorry if it was unclear that I added a new commit with my previous comment.

@arb arb added this to the 1.2.0 milestone Oct 31, 2014
arb added a commit that referenced this pull request Oct 31, 2014
Support for global view context
@arb arb merged commit 7c4b64e into hapijs:master Oct 31, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 31, 2014

Thanks @jagoda!

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

gergoerdosi commented Oct 31, 2014

@jagoda The "Global context" section in the view tutorial should be updated too: http://hapijs.com/tutorials/views#global-context

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 31, 2014

Should hold off on any documentation updates until the new version is published AND hapi uses it.

}
else {
defaultContext = this._context ? Hoek.clone(this._context) : {};
}

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 1, 2014

Member

This block should be a single line statement.

var defaultContext = (this._context ? (typeof this._context === 'function' ? this._context() : Hoek.clone(this._context)) : {});

This comment has been minimized.

Copy link
@jagoda

jagoda Nov 1, 2014

Author Contributor

OK. Is there something in the style guide that I missed that applies to this?

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 2, 2014

Member

Nope. just cleaner and most consistent.

@jagoda jagoda mentioned this pull request Nov 1, 2014
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Nov 1, 2014

@arb no problem. I'm happy to help with documentation when the time comes. @hueniverse I opened #7 to address the style issues.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 2, 2014

Feel free to open a PR on hapi with the updated doc and detailed change in the issue notes. The PR should include the change to package.json as well as npm-shrinkwrap.json.

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Nov 3, 2014

I'd be happy to help with the PR on Hapi once 1.2.0 is published.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 4, 2014

I need to fix master to remove the hapi 8 stuff I got in there by accident...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.