-
Notifications
You must be signed in to change notification settings - Fork 146
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
Supporting KoaJS middleware, (originally by @otothea) #129
base: master
Are you sure you want to change the base?
Conversation
Thank you. Great to see a PR for adding Koa support. However, I'd like to see correct naming ( |
Hello @xinbenlv have you checked that this code still works? I originally wrote it 4 or 5 years ago for a project I was working on in node 0.11 and have not edited it since. I imagine you would want to change it from |
@otothea seems to me it still works. I am currently using it, and it was able to identify client id just fine. And the diff between the function of @jtillmann Hi. thank you for being open minded. As in the description, I am not super saavy with KoaJS middleware nor the |
A few things.
I've tried to fix it up for you real quick. I don't use this package nor have I tested this code, I was made aware of this PR in the Koa slack channel and decided to take a quick stab at it. module.exports.koaMiddlewhere = function (tid, options) {
this.tid = tid;
this.options = options;
var cookieName = (this.options || {}).cookieName || "_ga";
return async function (ctx, next) {
ctx.request.visitor = module.exports.createFromSession(ctx.session);
if (ctx.request.visitor) {
return await next();
}
var cid;
if (ctx.cookies && ctx.cookies.get(cookieName)) {
var gaSplit = ctx.cookies.get(cookieName).split('.');
cid = gaSplit[2] + "." + gaSplit[3];
}
ctx.request.visitor = init(tid, cid, options);
if (ctx.session) {
ctx.session.cid = ctx.request.visitor.cid;
}
await next();
}
}; |
Thank you jeff @jeffijoe I wonder if too a larger extent, can we make it re-use / share code with what's in the express middleware so we don't duplicate code such as |
@xinbenlv that's on you, my work here is done. 😉 |
return async function (ctx, next) { | ||
ctx.request.visitor = module.exports.createFromSession(ctx.session); | ||
if (ctx.request.visitor) { | ||
return await next(); |
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.
No need to await promise before return when function is async.
return await next(); | |
return next(); |
Dear @otothea, @jtillmann and contributors of peaksandpies/universal-analytics
I know this is not the optimal solution, but I just want to present a solution to move forward discussion of #128 (And I am not savvy enough with Koa and generator function to make this better)
Is there any plan for KOA support?
Credit