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

Revert 9.x events changes #18942

Closed

Conversation

MylesBorins
Copy link
Contributor

It appears that 3ae5cf2 may have been Semver-Major as it broke the following code sample

var assert = require('assert')
var EventsModule = require('events')
var descriptor = Object.getOwnPropertyDescriptor(EventsModule, 'usingDomains')
assert(descriptor)

This PR reverts 3ae5cf2 and the two commits that were backported along side of it 28edc1d and 35471bc

We should likely get a new release out of the door asasp

/cc @watson

@nodejs-github-bot nodejs-github-bot added domain Issues and PRs related to the domain subsystem. events Issues and PRs related to the events subsystem / EventEmitter. v9.x labels Feb 22, 2018
@addaleax
Copy link
Member

Isn’t it, like, pretty trivial to just add back EventEmitter.usingDomains = false; instead?

@@ -31,6 +32,9 @@ module.exports = EventEmitter;
// Backwards-compat with node 0.10.x
EventEmitter.EventEmitter = EventEmitter;

EventEmitter.usingDomains = false;
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I’m basically suggesting only taking this line of the diff, and leaving the rest alone

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'm open to that as well, wasn't sure the context or why that line was removed. Should we add that line back to master instead?

Would doing another release next tuesday make sense then or should we get this out asap

Copy link
Member

Choose a reason for hiding this comment

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

@MylesBorins I guess it seems fair to remove the line because it would be false-y by default anyway, so explicitly setting it here and thereby leaking domains logic into this module seems undesirable.

Would doing another release next tuesday make sense then or should we get this out asap

It’s not really all that clear to me just how breaking this change is or how many people it would affect – but if we’re only adding this line back, I don’t see a reason not to do a release as soon as we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok opening a PR

@fhinkel
Copy link
Member

fhinkel commented Feb 22, 2018

+1 to @addaleax 's suggestion.

@vdeturckheim
Copy link
Member

+1 on @addaleax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain Issues and PRs related to the domain subsystem. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants