events: remove unneeded safety check code

[captainsafia]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

events

Description of change

In the process of creating #9865, it was discovered that the code
checking whether or not events was defined was unnecessary because
the only situation in which events would be undefined is if it is
monkeypatched by an external entity. This should be removed in order
to discourage this. If the test added in #9865 is merged, it will
need to removed on merge of this commit.

Before merging this change we might want to check and see if any
significant usage of monkeypatching events in the ecosystem.

[Trott]

Before merging this change we might want to check and see if any
significant usage of monkeypatching events in the ecosystem.

/cc @ChALkeR@addaleax

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/5038/

[Trott]

In theory, seems good to me, but I'd want to know if we can get a feel for if this is used in the ecosystem. I'd guess not but I've certainly been surprised in the past, so
¯\(ツ)/¯

[cjihrig]

I'm not so sure about this change. If we actually enforced that messing with underscored properties (_events in this case) voided the warranty, then this would be an easy thumbs up. But, we don't. I guess maybe @ChALkeR can tell us otherwise.

[mscdex]

I seem to recall some third party module using EventEmitter methods on a custom object (where _events was not pre-defined), that was the whole reason for adding these kinds of checks throughout the various EventEmitter methods in the first place.

[sam-github]

Its really common for code to derive from EventEmitter, but forget to call the EventEmitter constructor, so the only reason for these checks is not monkey patching. This is semver-major, and I'm -1 on it.

[Trott]

@sam-github Do you mean Foo inherits from EventEmitter (for example, via util.inherits()), and Foo has its own constructor that doesn't call the EventEmitter constructor, so the ._events property is undefined?

If so: @captainsafia Maybe the thing to do is close this and add the above situation to your test in #9865.

If not: @sam-github Can you demonstrate with sample code or something?

[captainsafia]

@sam-github: Thanks for adding in this insight!

@sam-github Do you mean Foo inherits from EventEmitter (for example, via util.inherits()), and Foo has its own constructor that doesn't call the EventEmitter constructor, so the ._events property is undefined?

@Trott: Yep. This is the case.

> const util = require('util'); > class MyStream{constructor(){} }; > util.inherit(MyStream, events.EventEmitter); > const s = new MyStream(); > s._events undefined 

Closing this in favor of augmenting the test suite introduced in #9865.