8000 using defunctzombie fork of uuid because node-uuid has been abandoned by developer by antialias · Pull Request #2182 · request/request · GitHub
[go: up one dir, main page]
Skip to content

using defunctzombie fork of uuid because node-uuid has been abandoned by developer #2182

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

using defunctzombie fork of uuid because node-uuid has been abandoned by developer #2182

wants to merge 1 commit into from

Conversation

antialias
Copy link
Contributor

No description provided.

@antialias antialias mentioned this pull request Apr 18, 2016
Closed
@simov
Copy link
Member
simov commented May 9, 2016

The critical bugs seems to be fixed now: https://github.com/broofa/node-uuid/issues/122 What's your stance on that?

@antialias
Copy link
Contributor Author

The author of node-uuid has admitted that he is an absentee landlord for his project, so I recommend moving away from it for that reason alone.

My personal motivation for switching request over to uuid comes from my desire to address the issue described in my still un-merged PR to node-uuid: https://github.com/broofa/node-uuid/pull/136

So, I still support merging this PR.

@simov
Copy link
Member
simov commented May 9, 2016

My main concern is this:

This branch is 33 commits ahead, 43 commits behind broofa:master.

It turns out that @coolaj86 is maintaining the broofa's version of uuid. So I'm wondering what exactly is the difference between the two modules.

@antialias
Copy link
Contributor Author
antialias commented May 9, 2016 •
edited
Loading

Additions to broofa/node-uuid that aren't in defunctzombie/node-uuid
(i.e. what's in the 43 commits behind): defunctzombie/node-uuid@defunctzombie:master...broofa:master

Additions to defunctzombie/node-uuid that aren't in broofa/node-uuid
(i.e. what's in the 33 commits ahead):
broofa/node-uuid@broofa:master...defunctzombie:master

In summary, the parts of broofa/node-uuid that were updated by the commits that aren't in defunctzombie/node-uuid were re-written in the defunctzombie branch and are no longer applicable.
Specific additions in the defunctzombie fork:

  • specs were ported over to be run directly in node, and integration with travis-ci was set up.
  • UMD boilerplate was removed along with code that makes (incorrect) assumptions about if we are running in a browser, node, or a bundling system. <-- this is good and makes the PR that I opened against @broofa's fork unnecessary.

More reasons to switch: node-http-signature, sockjs-node and vow-fs have all switched over to defunctzombie/node-uuid. request is the only remaining high-profile dependent of node-uuid that I could find that hasn't switched.

@defunctzombie
Copy link

It is absurd to have two packages doing the same thing. I originally forked cause node-uuid on npm was getting no updates. We need to merge this stuff back and just use the uuid package on npm. This is beyond confusing for no reason.

@antialias
Copy link
Contributor Author

bump

@antialias
Copy link
Contributor Author

bump
just keeping this on @simov's radar.

@antialias
Copy link
Contributor Author

bump @simov

@antialias
Copy link
Contributor Author

@simov what can I do to get traction on this PR? It's been almost three months since it was opened.

@simov
Copy link
Member
simov commented Jul 21, 2016

@antialias you can help the original uuid module and the forked one get merged back together.

@antialias
Copy link
Contributor Author
antialias commented Jul 21, 2016 •
edited
Loading

@simov: @defunctzombie's fork exists because @broofa no longer maintains node-uuid. I have tried contacting @broofa in several different PRs about this since at least February and he has been unresponsive so far. @broofa has even admitted to being an absentee landlord on node-uuid, see https://github.com/broofa/node-uuid/issues/116#issuecomment-156280048.

The actual issue that I'm trying to resolve here is that node-uuid misbehaves when run in node with jsdom globals. I get this errors from node-uuid because my packages depend on request indirectly:

[SECURITY] node-uuid: crypto not usable, falling back to insecure Math.random()

switching request to uuid fixes that.

@antialias antialias force-pushed the defunctzombie-uuid branch from 6726d15 to 263e16e Compare July 27, 2016 19:34
@vdh
Copy link
vdh commented Jul 27, 2016

you can help the original uuid module and the forked one get merged back together.

@simov: If the two modules ever did get merged, I would expect them to keep the more preferable uuid package name anyway.

@tracker1
Copy link

maybe @broofa should unpublish node-uuid (tongue in cheek).

@vdh
Copy link
vdh commented Jul 28, 2016

@tracker1 deprecation is the less-dramatic alternative 😉

@tracker1
Copy link

@vdh suggestion made... https://github.com/broofa/node-uuid/issues/142#issuecomment-236058539

@coolaj86
Copy link

Oddly enough, I liked node-uuid because of how simple the code was (I think it works in the browser without any build steps, but I can't remember).

I started taking it on and got it to "works for me" and the abandoned it. I could stand to check the issues and clean them up.

@vdh
Copy link
vdh commented Jul 29, 2016 •
edited
Loading

So with the unification discussion going on in https://github.com/broofa/node-uuid/issues/142, this PR is really more about whether or not request should switch their dependency over to the uuid package name, right?

Assuming that Since the two packages will eventually be unified as uuid, wouldn't it make sense to switch over to the version that has already fixed the https://github.com/broofa/node-uuid#136 issue?

@vdh vdh mentioned this pull request Jul 29, 2016
Closed
@marcoalbarelli marcoalbarelli mentioned this pull request Sep 5, 2016
Merged
@jonathanong
Copy link
jonathanong commented Oct 5, 2016 •
edited
Loading

in jest, i'm getting really annoying messages due to node-uuid:

    console.warn node_modules/node-uuid/uuid.js:48
      [SECURITY] node-uuid: crypto not usable, falling back to insecure Math.random()

node-uuid seems to be reading the environment as a browser environment even though i'm running the tests in node. this is not an issue with uuid.

please merge this so my test output isn't stupid!

@mikeal
Copy link
Member
mikeal commented Oct 5, 2016

Do we actually need RFC compliant UUID's?

We might be able to get away with just this:

let uuid = (a) => a?(a^Math.random()*16>>a/4).toString(16):([1e7]+-1e3+-4e3+-8e3+-1e11).replace(/[018]/g,b)

https://gist.github.com/jed/982883

@pcalves
Copy link
pcalves commented Oct 20, 2016

I'm getting the same output in Jest as @jonathanong due to this package's use of node-uuid.

Since the two packages will eventually be unified as uuid, wouldn't it make sense to switch over to the version that has already fixed the https://github.com/broofa/node-uuid#136 issue?

Any thoughts on this? It seems like the best solution/compromise until the whole debacle around node-uuid/uuid is resolved (if it hasn't been already).

@randunel
Copy link
randunel commented Nov 16, 2016 •
edited
Loading

node-uuid also has verbose output with --trace-sync-io:

(node:17836) WARNING: Detected use of sync API
    at _rng (node_modules/node-uuid/uuid.js:60:53)
    at v4 (node_modules/node-uuid/uuid.js:224:55)
    at Multipart (node_modules/request/lib/multipart.js:10:19)

Edit: added this comment to show support for dropping node-uuid completely as suggested by @mikeal

@vdh
Copy link
vdh commented Nov 17, 2016 •
edited
Loading

@randunel @mikeal Why mock it? uuid already exists and works, it's where the new releases of node-uuid are going to be published under, and the node-uuid name is going to be deprecated on NPM. Seems pretty straightforward.

@randunel
Copy link
randunel commented Nov 17, 2016 •
edited
Loading

@vdh existing node-uuid has at least 2 problems:

  • claims of abandonment (why this issue was created)
  • uses *Sync node.js api (why I support replacing it with the Math.random mock solution @mikeal proposed)

Edit: misread uuid as node-uuid, although they point to the same github repo, I haven't checked if uuid is still using *Sync calls.

@broofa
Copy link
broofa commented Nov 17, 2016

I've handed node-uuid off to @defunctzombie. He'll be merging the two projects.

@randunel: Regarding sync use of randomBytes, please chime in on uuidjs/uuid#150 if you have an explanation for why this is an issue.

@defunctzombie
Copy link

Following this thread has been rather amusing. Anyhow, hopefully this helps settle the back and forth.

I have deprecated on npm node-uuid and released a new version of uuid. The new repo lives here: https://github.com/kelektiv/node-uuid. Enjoy.

@SimenB
Copy link
Contributor
SimenB commented Nov 18, 2016

@simov could you merge this now, as the deprecation warnings have started coming in 😄

@simov simov mentioned this pull request Nov 18, 2016
Merged
@simov simov closed this in #2467 Nov 18, 2016
@monolithed
Copy link

Why is not published yet?

@skarbovskiy skarbovskiy mentioned this pull request Jul 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0