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

why don't use uuid module instead of uid2? #49

Closed
LeoIannacone opened this issue Jun 17, 2014 · 9 comments
Closed

why don't use uuid module instead of uid2? #49

LeoIannacone opened this issue Jun 17, 2014 · 9 comments
Labels

Comments

@LeoIannacone
Copy link

Hi,

It would be nice use node-uuid module instead of uid2.
It's more robust (I think) and more RFC compliance...

Homepage: https://github.com/broofa/node-uuid

@LeoIannacone
Copy link
Author

This patch seems work fine:

diff --git a/index.js b/index.js
index d48bcd1..9cae893 100644
--- a/index.js
+++ b/index.js
@@ -9,7 +9,7 @@
  * Module dependencies.
  */

-var uid = require('uid2')
+var uuid = require('node-uuid')
   , onHeaders = require('on-headers')
   , crc32 = require('buffer-crc32')
   , parse = require('url').parse
@@ -85,7 +85,7 @@ function session(options){

   // generates the new session
   store.generate = function(req){
-    req.sessionID = uid(24);
+    req.sessionID = uuid.v4();
     req.session = new Session(req);
     req.session.cookie = new Cookie(cookie);
   };
diff --git a/package.json b/package.json
index df44c60..198f1b1 100644
--- a/package.json
+++ b/package.json
@@ -11,7 +11,7 @@
     "cookie-signature": "1.0.3",
     "debug": "1.0.2",
     "on-headers": "0.0.0",
-    "uid2": "0.0.3",
+    "uuid": "*",
     "utils-merge": "1.0.0"
   },
   "devDependencies": {

@jonathanong
Copy link
Member

Why does this matter?

@dougwilson
Copy link
Contributor

I don't think it does. Plus there is no RFC for how to store session IDs in cookies.

@LeoIannacone
Copy link
Author

In debian we have node-uuid, it's used by other 880 modules (uid2 by only 45) according with npm.
We consider it as a better implementation, I would suggest you to use it. That's all.

@dougwilson
Copy link
Contributor

There are 880 modules that generate sessions with node-uuid? I think those numbers are not actually useful here. If we created a module that generated super awesome secure and fully random values and it was only used by this module, then your numbers argument would say it is inferior simply because it's only used by 1 module.

We can replace it. Can you provide us an actual reason why to replace it? So far you have not really provided a reason, and without a good maintainer for this module, we don't want to really just willy-nilly change stuff.

@Fishrock123
Copy link
Contributor

@dougwilson But there is an RFC on uid's http://www.ietf.org/rfc/rfc4122.txt

uid2 has no repository and no tests, so I'm not exactly in favor with it although it is quite simple and does the job.

@dougwilson
Copy link
Contributor

@Fishrock123 the original reasoning on using UUID was it is "and more RFC compliance", but how it is more RFC compliance than using uid? I know there is a RFC for what a UUID is, but that doesn't related to using it for session IDs, which is my point.

@jonathanong
Copy link
Member

Going to just close. Reopen if there's a valid reason unless someone wants to maintain this

dougwilson added a commit that referenced this issue Jun 18, 2014
@dougwilson
Copy link
Contributor

With the next version of the library, feel free to do this yourself with the genid option:

app.use(session({
  genid: function(){ return uuid.v4() }
}))

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

No branches or pull requests

4 participants