chore(lib): don't require domain by default #101
Conversation
Domains have an unwanted overhead on Node.js v12. The overhead is observable just by requiring `domain`. To prevent that, we postpone requiring `domain` until we are sure it's necessary (in which case it was already required somewhere else).
mmarchini
force-pushed
the
dont-require-domain
branch
from
830b004 to
cb1ef62
Compare
| @@ -157,8 +161,17 @@ function _getSerializedContext(err) { | |||
| _.omit(err, self._knownFields) : | |||
| {}; | |||
|
|
|||
| if (topLevelFields.domain instanceof domain.Domain) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Although 8.x goes EOL later this month, so we can drop that soon...
| // 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) { |
There was a problem hiding this comment.
I think we can load this even more lazily by loading the domain module only when processing a .domain property on the topLevelFields object.
There was a problem hiding this comment.
We could, but if we get here domains were already loaded somewhere else. Doing it here saves us a hasOwnProperty/in.
|
lgtm! |
|
Thanks! Released as |
Domains have an unwanted overhead on Node.js v12. The overhead is
observable just by requiring
domain. To prevent that, we postponerequiring
domainuntil we are sure it's necessary (in which case itwas already required somewhere else).