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

fix(uuid): import versioned uuid #2996

Merged
merged 1 commit into from Aug 6, 2018
Merged

fix(uuid): import versioned uuid #2996

merged 1 commit into from Aug 6, 2018

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Aug 5, 2018

PR Checklist:

  • I have run npm test locally and all tests are passing.
  • I have added/updated tests for any new behavior.
  • If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A, or add link to issue here]

PR Description

This PR proposes way to import uuid module, per deprecation notice (https://github.com/kelektiv/node-uuid#uuid-) to not to import top level import directly. Currently top level import provides compatibility (https://github.com/kelektiv/node-uuid/blob/fe4ae79c55af2c7cbf2bb39d3bcb6716d5367091/index.js#L4) to forward to v4, but also it imports v1 still.

This behavior is generally not a major concern, but few framework specifically like Electron (uuidjs/uuid#189 / electron/electron#2073 ) have issues with crypto performance, so v1's up front invocation rng costs amount of time on windows platform.

Since top level import already points to v4, this change shouldn't cause any functional differences.

@simov
Copy link
Member

simov commented Aug 6, 2018

This seems to have been fixed long time ago uuidjs/uuid@501a8f4 so I don't see how changing the import now would make any difference.

@kwonoj
Copy link
Contributor Author

kwonoj commented Aug 6, 2018

@simov current version request uses (uuid@^3.1.0) still invokes rng() upfront: https://github.com/kelektiv/node-uuid/blob/c50ac88f098ecfbff9a940816c8e6825ffd7e05a/v1.js#L10 , I think this PR is still useful to let request ensures to use latest uuid include those fixes.

Importing v4 specific is additional changes along with bump follows deprecation notices.

@simov
Copy link
Member

simov commented Aug 6, 2018

Now I see the deprecation bits in the docs. Request does not call the rng() function upfront because ^ bumps to the latest minor.

Either way it's good to have the versioned import here 👍

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.

None yet

2 participants