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

expose serverProperties in the connection object #452

Merged
merged 1 commit into from
Oct 12, 2018

Conversation

jfromaniello
Copy link
Contributor

Sometimes is useful to check some properties of the broker.

amqplib for python has this:
https://stackoverflow.com/a/7599072

Copy link
Collaborator

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Yup, this looks useful. Thanks!

test/connect.js Outdated
var config = parts.query || {};
connect(config, {}, (err, connection) => {
if (err) { return done(err); }
console.dir(connection);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this debug output?

Copy link
Collaborator

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Ops, didn't notice there's a new-style function definition, which won't run in older Node.JS (with which this lib still claims compatibility).

test/connect.js Outdated
var url = require('url');
var parts = url.parse(URL, true);
var config = parts.query || {};
connect(config, {}, (err, connection) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah -- CI is failing because this won't parse in older versions of Node.JS.

@kibertoad
Copy link
Collaborator

@squaremo Speaking of compatibility - are there plans to ever drop support for at least the hopelessly old versions (< 4)?

@squaremo
Copy link
Collaborator

squaremo commented Oct 8, 2018

Speaking of compatibility - are there plans to ever drop support for at least the hopelessly old versions (< 4)?

@kibertoad It is tempting, especially as it's getting difficult to keep Node v10 happy without breaking things for those older versions. https://nodejs.org/metrics/ seems to indicate that versions before v4 are still in wide use, however.

@kibertoad
Copy link
Collaborator

@squaremo I would argue that these numbers are pretty skewed by many popular libraries (that run CI) still supporting older versions and running tests against them; when you factor in statistics that rely on people actually providing input, numbers are very different: https://nodejs.org/en/user-survey-report/#version
Moreover, main reason why developers don't upgrade Node.js is "it's already working, don't touch anything" kind of mentality for fragile super-legacy systems. For such cases it's irrelevant whether or not there are updates for dependencies that they are using, because they are most likely not going to update anyway.

@jfromaniello
Copy link
Contributor Author

Thank you, applied your feedback:

  1. removed the console.dir I accidentally commited
  2. changed the arrow function to function

@cressie176
Copy link
Collaborator

Re upgrading

With the increased emphasis on security from GitHub and npm (via audit), it's becoming harder and harder to support older node versions. It only takes a single dependency to introduce a newer node feature, then require a security patch, to force your hand.

@kibertoad
Copy link
Collaborator

I would suggest to continue discussion of whether or not it is preferrable to drop support for older Node.js versions here: #463

Copy link
Collaborator

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Nice and simple, I like. Thanks for adding a test too :-)

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

4 participants