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

Supporting KoaJS middleware, (originally by @otothea) #129

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

xinbenlv
Copy link

@xinbenlv xinbenlv commented Mar 11, 2019

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

@jtillmann
Copy link
Member

Thank you. Great to see a PR for adding Koa support. However, I'd like to see correct naming (koaMiddleware instead of koa_middlewhere), documentation, and tests before merging it.

@otothea
Copy link

otothea commented Mar 11, 2019

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 */yield to async/await at this point?

@xinbenlv
Copy link
Author

xinbenlv commented Mar 11, 2019

@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 koa_middlewhere and middlewhere is still exactly the same since ~5 years ago when you first wrote it. Since KoaJS has increasingly become popular, your commit will make this NPM package greatly more useful to many, you deserve to take the credit, do you want to create a follow up PR? Feel free to start your PR, I can withdraw mine. If you don't have time, I can try to learn about it and may need some of your(@otothea) advice regarding KoaJS's middleware as well as @jtillmann's advice on Measurement Protocol and getting the client id etc.

@jtillmann Hi. thank you for being open minded. As in the description, I am not super saavy with KoaJS middleware nor the Google Analytics Measurement Protocol that this universal-analytics uses. If @otothea is willing to take time to look into it and create his/her authored PR, I can facilitate. Otherwise, I can spend sometime looking into it to add tests and documentation. I will need your help and advice though.

@xinbenlv xinbenlv changed the title Apply @otothea 's commit 797d678a69517c624d1390f6c736596e980335d1 Supporting KoaJS middleware, (originally by @otothea 's commit 797d678a69517c624d1390f6c736596e980335d1) Mar 11, 2019
@xinbenlv xinbenlv changed the title Supporting KoaJS middleware, (originally by @otothea 's commit 797d678a69517c624d1390f6c736596e980335d1) Supporting KoaJS middleware, (originally by @otothea) Mar 11, 2019
@jeffijoe
Copy link

A few things.

  • You should use async await, generators are not supported anymore.
  • You should use ctx.request, req is the low-level HTTP request, and request is Koa's request.

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();
  }
};

@otothea
Copy link

otothea commented Mar 12, 2019

@xinbenlv I would go with what @jeffijoe is suggesting above as I've not been working in koa for a long time and don't know the best practices. I don't need credit, if you go through testing and submit a PR you will have done much more work than I ever did xD

@xinbenlv
Copy link
Author

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 var cookieName = (this.options || {}).cookieName || "_ga";

@jeffijoe
Copy link

@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();
Copy link

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.

Suggested change
return await next();
return next();

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.

5 participants