oskosk / express-socket.io-session

Share a cookie-based express-session middleware with socket.io
https://www.npmjs.com/package/express-socket.io-session
MIT License
135 stars 14 forks source link

Some recommend NOT USING this module (Please Read) #38

Open knoxcard opened 6 years ago

knoxcard commented 6 years ago

EDIT by the module's author: The author of this issue makes a point somehow but the StackOverflow reference they left here is older than this module. Please, continue reading and come up with your own criteria. -- @oskosk.

The original text of the issue follows:

Just posting this to save everyone some time!

This module might not fit your particular needs. There is a standard way to use Socket.io in conjunction with Express.

https://stackoverflow.com/questions/25532692/how-to-share-sessions-with-socket-io-1-x-and-express-4-x

The solution is surprisingly simple. It's just not very well documented.

sio.use(function(socket, next) {
    // sessionMiddleware(socket.request, socket.request.res, next);  <-- IMPORTANT: this was giving me an error...

    // change it to this...
    sessionMiddleware(socket.request, {}, next);  
});

Here's a full example with express 4.x, Socket.IO 1.x and Redis:

var express = require("express");
var Server = require("http").Server;
var session = require("express-session");
var RedisStore = require("connect-redis")(session);

var app = express();
var server = Server(app);
var sio = require("socket.io")(server);

var sessionMiddleware = session({
    store: new RedisStore({}), // XXX redis server config
    secret: "keyboard cat",
});

sio.use(function(socket, next) {
   //     sessionMiddleware(socket.request, socket.request.res, next);
    sessionMiddleware(socket.request, {}, next);
});

app.use(sessionMiddleware);

app.get("/", function(req, res){
    console.log(req.session) // Session object in a normal request
});

sio.sockets.on("connection", function(socket) {
  console.log(socket.request.session) // Now it's available from Socket.IO sockets too! Win!
});

server.listen(8080);
knoxcard commented 6 years ago

If you want socket.io to overwrite or create a new session variable, here is this example...

Bonus code included as well, get subdomain! For subdomains you will need...https://github.com/bmullan91/express-subdomain

This example sends the entire user session as JSON back to the client. Very useful for one page apps. Might want to remove certain sensitive items such as the entire session cookie being sent back, all up to you.

      app.sio.use((socket, next) => {
        var socket_subdomain = socket.handshake.headers.host.split('.')[0]
        // // console.log('socket subdomain: ' + socket_subdomain)
        sessionMiddleware(socket.handshake, {}, err => {
          var session = socket.handshake.session
          session.user_id = 1125
          session.save()
          session.reload(err => {
            app.sio.sockets.in('room_' + session.id).emit('auth', session)
          })
        })
        sessionMiddleware(socket.request, {}, next)
      })
daverickdunn commented 6 years ago

I have to second this. I spent "countless hours" trying to get this package to behave correctly. It's a waste of time. All that's required is the solution in the linked SO post. Works a charm...

oskosk commented 6 years ago

I'm sorry folks... It can be updated if you wish. PRs are welcome always. Don't be angry.

If you check the README.md here, you can find a section tilted Inspiration with SO links to where I got started too. This is a module written for my convenience at the time, for simplifying my use case (which involved more that one developer and more than one codebase that used this module).

I wrote it after some digging on SO, as you probably did. I wrote it for me and it worked. I think it works for some use cases.

This StackOverflow answer you mentioned, I've read it a few times. It's an answer from September 2014 and I started this module on November 2014 iirc. And from that moment, this module started collecting something like 3 or 4 features on top of what you can achieve with that snippet in the SO link you propose as solution.

At some point in life, even as soon as I published this in Github I wrote a post about how I solved it first... Which is basically, the answer you link to... and only after the whole explanation, I mention that I wrote a module for simplifying some parts.

Also, if you continue reading, the StackOverflow thread, you can see a lot of feedback on other comments saying that the proposed solution no longer works.

Maybe this module has bugs too and needs to be updated. I'm no longer a frequent user of it as I was before.

...I don't know, don't be angry.

daverickdunn commented 6 years ago

@oskosk I don't think either of us are angry, we're just trying to save others some time. I'd make a PR if I could figure out what was wrong, but the SO post works well for me, albeit with a few extra lines to handle cookies.

knoxcard commented 6 years ago

@oskosk - I am definitely not angry and never was. I am sorry if my post was harsh... I put it in all CAPS to generate attention, as I felt it was important.

Here's how I look at it, the last version release for this package was on May 6, 2015, 3 years ago? So I felt like this project was not being maintained or possibly abandoned. Sometimes, things simply change and some npm packages become obsolete because other packages integrate your code and then make additional changes on top of that.

If you can accomplish your objective with just three lines of code as opposed to fifteen and your code will even work going forward after upgrades, why not do just that?

You most likely helped Socket.io and Express push forward with this, but what I see at this point is them moving forward without the need for third party additions. That's all. Thank you for contributing this module, I did use this at first a while back and it was incredibly helpful!

oskosk commented 6 years ago

Thanks for the replies @knoxcard @daverickdunn. I think you may be confused about the point of this module. It's not to help one achieve what was not possible at some point in time. It's there to simplify everything to a one-line/one-sentence.

Internally it does the same thing that you see in the SO.

What feels weird to me is that you still mention that some thing does not work with this module and some other thing works with that SO snippet, when the SO answer you're sticking to is from 2014, written before this module was created and even non functional currently as all of the other answers there mention. There's even people recommending this module after the snippet stopped working for them...

What this module simplified at the time (and now too probably) is the following.

  1. Have a way to import the module.
  2. Use one single line for your already working express + express session. THIS ONLY HAPPENS TO BE SIMPLER if you already have an express with session working. If you are there to write everything while learning and discovering what Socket IO and express are... you will still struggle...

That was always the point and it worked and works IMO... When you continue the path of the SO snippet you will be the author of this glue... It will take more than a few dozens of lines... you will probably end up factoring out that piece of code and using it by importing it in another file (hopefully for clearness)...

This is what I did here.

I really appreciate your feedback. Since you wrote this, I tested the module again and it worked. The example worked and I just attempted to make it work with another piece of code from scratch...

I'll be even glad to reopen the issue, but you, as the author of the issue haven't really specified what is it that does not work... And for me, being the author of the code here, it's hard to come up with whatever is needed to make it work...

Even if it's the case that new versions of Express, express-session or Socket.IO started being incompatible with this module... The info was not left here in this issue, so it's hard to work on that.

oskosk commented 6 years ago

I think I found a problem with the logic of the autoSave option provided by this module.

I'm double checking now.

oskosk commented 6 years ago

This problem is fixed by #40 now and by the published 1.3.4

oskosk commented 6 years ago

I think I'm gonna reopen this issue and leave some notes. It's not a lesser problem if people spend a lot of time attempting to solve a problem with this module and not achieving it...

But I do recommend to provide a better reference than the 2014 Stack overflow answer. i'm gonna update the title to be less opinionanted

oskosk commented 6 years ago

I'm hereby reopening the issue. Hope you don't mind the change in the title and the notice I left on this issue's description.

knoxcard commented 6 years ago

@oskosk - awesome! I see you issued a new version release and replied to that pull request in regards to auto session reload(). My apologies again for how I approached this. After reading your statements, I was able to better recognize the situation and your true intent for this module.

Going forward, I should probably reach out to project maintainers first to garner feedback about particular issues. Then open a thread where everyone can discuss in a civilized manner expressing their viewpoints. You responded to my post fairly quickly expressing concern, which shows that you care. Thanks for caring and all the great work!