Node: Is overriding protection for .off appropriate?

[Ulmanb]

Hey, Follow up on #17156 ,
Do you think a PR on the matter of overriding protection for .off is appropriate, and if so, what scenarios should be regarded?

[addaleax]

What would the motivation for forbidding overriding be?

[benjamingr]

Copying comment by @ChALkeR in this comment:

Everyone, do you think if an attempt to override it should _warn_ users? Such overriding would mean one of two things:

1. It's being overriden with an exact same alias. This case is fixeable in userland with a simple check in the lib that tries to set the alias.
2. It's being overriden with something else — e.g. a variable or a method which does other things. This case means that something is either broken or is probable to be broken — e.g. (in variable case) if userland code later checks for the .off variable and treats it as a flag, or (in different method case) if some userland code (e.g. different libs) expect different behaviour from .off — Node.js one and overriden one.

---

See the rest of the discussion there. Pinging people who weighed in @ChALkeR @ljharb @BridgeAR

[ljharb]

I think a simple warning when .off is overridden with anything besides removeEventListener (the current value) should suffice. When it’s overridden with the correct value, i think it’s fine and no warning is needed.

[addaleax]

@benjamingr It isn’t quite clear from that comment, but I think that means what you are talking about is .off being set as a property on an EventEmitter instance, not overriding EventEmitter.prototype.off itself (which is what I thought when I read “overriding”)… is that correct?

[ljharb]

My interpretation was about monkeypatching the prototype; it’s perfectly fine for someone to do anything they like to the instance.

[addaleax]

It’s weird, but my thinking would be reversed I guess? Maybe there’s something special about off that I’m not seeing, but generally, monkeypatching seems like something that should be allowed?

Since the API here is well-defined, it should be safe for userland to override it as long as they know they are overriding it, right?

I can see how people might be surprised to find out that this.off = false; would start to override an EventEmitter method, so a warning in that case seems fine?

[ljharb]

Shadowing a prototype method on an instance only risks surprising/breaking the tiny segment of code that interacts with it, which will likely be located nearby. Monkeypatching a prototype affects all code in the entire process.

[TimothyGu]

I’m okay with making off a setter that’ll warn if the prototype’s method gets overridden.

[mcollina]

I'm -1 in adding override protection or emitting a warning. I think it's good as it is.

[bnoordhuis]

What is the conclusion here? Can this issue be closed?