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

chore(lib): don't require domain by default #101

Merged
merged 1 commit into from
Dec 13, 2019
Merged

chore(lib): don't require domain by default #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions lib/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@

// external modules
var assert = require('assert-plus');
var EventEmitter = require('events');
var _ = require('lodash');
var nerror = require('@netflix/nerror');
var domain = require('domain');
var MultiError = nerror.MultiError;
var VError = nerror.VError;
var safeJsonStringify;

// We load the domain module lazily to avoid performance regression on Node.js
// v12.
var domain;

// try to require optional dependency
try {
// eslint-disable-next-line global-require
Expand Down Expand Up @@ -157,8 +161,17 @@ function _getSerializedContext(err) {
_.omit(err, self._knownFields) :
{};

if (topLevelFields.domain instanceof domain.Domain) {
Copy link

Choose a reason for hiding this comment

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

This may not be necessary anymore now that Node.js sets the domain property on objects as being non-enumerable for versions of node >= 12.0.0

Copy link

Choose a reason for hiding this comment

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

I guess we still need to support Node.js >= 10.x, and possibly older versions (like 8.x as indicated by the Travis config) though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we should keep supporting all Node.js versions which are still active (8.x, 10.x, 12.x and 13.x)? If that's the case, it will take some time for us to leverage the changes introduced on v12 (out of curiosity, those changes are also part of the performance issues I found).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although 8.x goes EOL later this month, so we can drop that soon...

topLevelFields = _.omit(topLevelFields, [ 'domain' ]);
// We don't want to load domains just to check if topLevelFields.domain is
// a Domain instance, so first we make sure domains are already loaded.
if (EventEmitter.usingDomains) {
if (!domain) {
Copy link

Choose a reason for hiding this comment

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

I think we can load this even more lazily by loading the domain module only when processing a .domain property on the topLevelFields object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but if we get here domains were already loaded somewhere else. Doing it here saves us a hasOwnProperty/in.

// eslint-disable-next-line global-require
domain = require('domain');
}

if (topLevelFields.domain instanceof domain.Domain) {
topLevelFields = _.omit(topLevelFields, [ 'domain' ]);
}
}

// combine all fields into a pojo, and serialize
Expand Down
3 changes: 2 additions & 1 deletion test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

// core modules
var http = require('http');
var domain = require('domain');

// userland
var assert = require('chai').assert;
Expand Down Expand Up @@ -1002,6 +1001,8 @@ describe('restify-errors node module.', function() {

// eslint-disable-next-line max-len
it('should not serialize domain when using node domains', function(done) {
// eslint-disable-next-line global-require
var domain = require('domain');
var dom = domain.create();
dom.on('error', function(err1) {
var serializer = restifyErrors.bunyanSerializer.create({
Expand Down