-
Notifications
You must be signed in to change notification settings - Fork 214
add express response.locals object to the job context object #112
base: master
Are you sure you want to change the base?
Conversation
7a6af46
to
25b7921
Compare
25b7921
to
8467b13
Compare
rebased. @schleyfox @goatslacker anything I can do to help nudge this along to be merged? Thanks! |
8467b13
to
b40a1fb
Compare
@schleyfox @goatslacker Rebased again. ping :) |
96b8be7
to
2d96928
Compare
@@ -96,6 +96,7 @@ class BatchManager { | |||
duration: null, | |||
html: null, | |||
returnMeta: {}, | |||
resLocals: response.locals, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also include request and app level locals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specifically, should we just spread baseContext
into here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing in request
/ response
sounds good!
Could pass this is in explicitly - spreading in baseContext
sounds good too, at the risk of potentially increasing the public api surface area.
I'll update the PR to do this.
(Side note - unclear what batchMeta
is used for? Can this be removed?)
╔═ markl@dev21-uswest1cdevc: ~/oss/hypernova git:(add_response_object_to_context)
╚═ ♪ git --no-pager grep batchMeta
src/utils/BatchManager.js:75: batchMeta: {},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just spread baseContext into here?
We could change line 147 to be
const context = this.getRequestContext(null, token);
It would be a breaking change but then the context passed in would have access to baseContext
as well.
The downside is that the whole response object would be made available which means that consumers may be able to hijack the response.
What's an example of this? Maybe we can use this to create a test case for the feature. |
@goatslacker Thanks for the feedback!
We have a few logging middlewares that run at the end of the request. As part of a profiling thing we're doing, we want to include in the log - did the request hit a cold bundle cache or not? Erring on the side of being overly-verbose, I'll share our sample usage: So right now, using my forked version, a simplified version of our code looks something like // (CRS = "component rendering service")
// A home for all the stuff that CRS adds to the response object per component token
type CRSComponentContext = {
// Did the component request hit a cold cache?
wasBundleInCache?: boolean,
};
export default async function getComponent(
requestString: string,
context: Object,
) {
context.resLocals.crs = context.resLocals.crs || {};
const componentContexts = (context.resLocals.crs: { [token: string]: CRSComponentContext });
componentContexts[context.token] = {};
const componentContext = componentContexts[context.token];
// Decode requestString to get bundle/component info
const componentRequest = getComponentInfo(requestString);
// Fetch bundle from memory / disk / s3
const [bundle, wasBundleInCache] = await getBundle(componentRequest.service, componentRequest.sha);
componentContext.wasBundleInCache = wasBundleInCache;
const component = bundle.default[componentRequest.name];
return component;
} This allows us in a middleware later on to access (For example, somewhere else, in a logging middleware, we do something like) // (uses https://www.npmjs.com/package/tweenz)
export default function AccessLog () {
return async (requestDetails, req, res) => {
// wait for request to complete
await requestDetails;
const componentRequests = Object.keys(hypernovaRequest).map(key => {
const componentContext = res.locals.crs[key];
return {
// we'd also actually log some other stuff here too like timing, status etc
...componentContext,
}
});
logSomewhere(componentRequests);
};
}; |
Addresses #111
Motivation: To be able to pass things from inside
getComponent
to the rest of the express application (for custom middlewares)(poked around in
test/BatchManager-test.js
, but can't see a place we're already asserting things about what is on the context object that we're passing togetComponent
)Thanks!
@goatslacker