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

remove p-defer and use crypto.randomUUID if available #38

Closed
wants to merge 2 commits into from

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Nov 7, 2022

No description provided.

@rluvaton rluvaton changed the title remove p-defer and used crypto.randomUUID if available remove p-defer and use crypto.randomUUID if available Nov 7, 2022
@@ -8,6 +8,17 @@ const emptyLogger = {
log: empty
};

let uuidV4;
const crypto = require('crypto');
Copy link
Member

@shamil shamil Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't use uuid from crypto package yet, since this module still retains support for node 12

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why I check in line 14 if the function is defined...

Copy link
Member

@shamil shamil Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, so if you keep the uuid package anyway, why doing this magic, lets stick with single version and once we deprecate node 12 we will move to version from crypto package and remove the uuid module. I prefer to stay consistent between versions, the performance gain is not that crucual.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The randomUUID is 12 times faster but as you wish...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rluvaton just update to uuid version 9.. it will crypto.randomUUID if it's available (uuidjs/uuid#600)

@@ -29,7 +29,6 @@
"homepage": "https://github.com/bringg/node-arnavmq#readme",
"dependencies": {
"amqplib": "^0.10.3",
"p-defer": "^3.0.0",
Copy link
Member

@shamil shamil Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we actually had an own version of this, but decided to use this package to avoid managing custom version, so please lets keep it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No management is required in this it's just 4 lines of simple code...

Copy link
Member

@shamil shamil Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as explained earlier we had this already and it was decided to remove from the codebase, p-defer is commonly used package and downloaded 8 million times a day. Lets please stick with it.

@rluvaton rluvaton assigned shamil and unassigned rluvaton Nov 7, 2022
@@ -8,6 +8,17 @@ const emptyLogger = {
log: empty
};

let uuidV4;
const crypto = require('crypto');
Copy link
Member

@shamil shamil Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, so if you keep the uuid package anyway, why doing this magic, lets stick with single version and once we deprecate node 12 we will move to version from crypto package and remove the uuid module. I prefer to stay consistent between versions, the performance gain is not that crucual.

@rluvaton rluvaton closed this Nov 7, 2022
@rluvaton rluvaton deleted the remove-p-defer-and-try-to-use-crypto branch November 7, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants