Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

Adds the app object at hook.app. #34

Merged
merged 1 commit into from
Jan 23, 2016

Conversation

marshallswain
Copy link
Member

I think this is the most elegant place to add the app object so that it's always available in hooks, even on internal requests (which don't pass through one of the providers).

The next issue that will surface is probably circular requests where service A requests data from B, B from C, and C from A.

@marshallswain
Copy link
Member Author

The only changes in before.js are at line 4 where it's all wrapped in a function in order to pass in the app, and line 26 where it adds app to the hookObject.

@ekryski
Copy link
Member

ekryski commented Jan 23, 2016

Looks pretty good. Do you think we need the same thing in the after hooks? I think so, as you may want to check a config value and then behave differently based on whether that value is set or not.

I think this is the most elegant place to add the app object so that it's always available, even on internal requests that don't pass through one of the providers.

The next issue that will surface is probably circular requests where service A requests data from B, B from C, and C from A.
@marshallswain
Copy link
Member Author

I actually started doing that, then noticed that the hookObject gets copied from the before hooks to the after hooks. I've added a test to make sure the app object is present in afterHooks.

marshallswain added a commit that referenced this pull request Jan 23, 2016
@marshallswain marshallswain merged commit bbf97bd into master Jan 23, 2016
@marshallswain marshallswain deleted the make-app-available-in-hooks branch January 23, 2016 18:34
@daffl
Copy link
Member

daffl commented Jan 23, 2016

Where is the documentation? ;)

@marshallswain
Copy link
Member Author

Good point! I'll get that added.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants