Repository navigation
Is overriding protection for .off appropriate? #17844
Description
Activity
What would the motivation for forbidding overriding be?
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:
- 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.
- 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
.offvariable 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
Reacted by Nikita SkovorodaI 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.
@benjamingr It isn’t quite clear from that comment, but I think that means what you are talking about is
.offbeing set as a property on an EventEmitter instance, not overridingEventEmitter.prototype.offitself (which is what I thought when I read “overriding”)… is that correct?My interpretation was about monkeypatching the prototype; it’s perfectly fine for someone to do anything they like to the instance.
It’s weird, but my thinking would be reversed I guess? Maybe there’s something special about
offthat 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 anEventEmittermethod, so a warning in that case seems fine?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.
I’m okay with making
offa setter that’ll warn if the prototype’s method gets overridden.Reacted by Jordan Harband and Nikita Skovoroda- added a commit that references this issue
on Dec 26, 2017 I'm -1 in adding override protection or emitting a warning. I think it's good as it is.
What is the conclusion here? Can this issue be closed?
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?