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.

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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ee1445
remove p-defer and used crypto.randomUUID if available
rluvaton Nov 7, 2022
0c660ed
fix uuid
rluvaton Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
@@ -1,6 +1,6 @@
{
"name": "arnavmq",
"version": "0.12.0",
"version": "0.12.1",
"description": "ArnavMQ is a RabbitMQ wrapper",
"keywords": [
"rabbitmq",
Expand Down Expand Up @@ -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 •
edited

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 •
edited

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.

"serialize-error": "^8.0.1",
"uuid": "^9.0.0"
},
Expand Down
3 changes: 1 addition & 2 deletions src/index.js
View file
Open in desktop
@@ -1,4 +1,3 @@
const uuid = require('uuid');
const utils = require('./modules/utils');
const connection = require('./modules/connection');
const { ARNAVMQ_TRANSPORT_LOGGER_DEPRECATED } = require('./modules/warnings');
Expand Down Expand Up @@ -29,7 +28,7 @@ module.exports = (config) => {
consumerSuffix: '',

// generate a hostname so we can track this connection on the broker (rabbitmq management plugin)
hostname: process.env.HOSTNAME || process.env.USER || uuid.v4(),
hostname: process.env.HOSTNAME || process.env.USER || utils.uuidV4(),

// Deprecated. Use 'logger' instead. The transport to use to debug. If provided, arnavmq will show some logs
transport: utils.emptyLogger,
Expand Down
15 changes: 9 additions & 6 deletions src/modules/producer.js
View file
Open in desktop
@@ -1,5 +1,3 @@
const uuid = require('uuid');
const pDefer = require('p-defer');
const utils = require('./utils');
const parsers = require('./message-parsers');
const { ARNAVMQ_MSG_TIMEOUT_DEPRECATED } = require('./warnings');
Expand Down Expand Up @@ -169,7 +167,7 @@ class Producer {
if (options.rpc) {
return this.createRpcQueue(queue).then(() => {
// generates a correlationId (random uuid) so we know which callback to execute on received response
const corrId = uuid.v4();
const corrId = utils.uuidV4();
options.correlationId = corrId;
// reply to us if you receive this message!
options.replyTo = this.amqpRPCQueues[queue].queue;
Expand All @@ -191,16 +189,21 @@ class Producer {

this.publishOrSendToQueue(queue, msg, options);
// defered promise that will resolve when response is received
const responsePromise = pDefer();
this.amqpRPCQueues[queue][corrId] = responsePromise;

const prCallbacks = {};
const promise = new Promise((resolve, reject) => {
prCallbacks.resolve = resolve;
prCallbacks.reject = reject;
});
this.amqpRPCQueues[queue][corrId] = prCallbacks;

// Using given timeout or default one
const timeout = options.expiration || 0;
if (timeout > 0) {
this.prepareTimeoutRpc(queue, corrId, timeout);
}

return responsePromise.promise;
return promise;
});
}

Expand Down
15 changes: 14 additions & 1 deletion src/modules/utils.js
View file
Open in desktop
Expand Up @@ -8,6 +8,17 @@ const emptyLogger = {
log: empty
};

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

@shamil shamil Nov 7, 2022 •
edited

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 •
edited

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)


if (typeof crypto.randomUUID === 'function') {
uuidV4 = () => crypto.randomUUID();
} else {
// eslint-disable-next-line global-require
const uuid = require('uuid');
uuidV4 = uuid.v4;
}

module.exports = {
/**
* Default transport to prevent any printing in the terminal
Expand Down Expand Up @@ -44,5 +55,7 @@ module.exports = {
emitWarn.warned[code] = true;
process.emitWarning(message, { code, detail });
}
}
},

uuidV4
};