Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Domain feature must not clobber EventEmitter#domain field. #5310

Closed
cstockton opened this issue Apr 16, 2013 · 3 comments
Closed

Domain feature must not clobber EventEmitter#domain field. #5310

cstockton opened this issue Apr 16, 2013 · 3 comments

Comments

@cstockton
Copy link

I added a comment to the issue here: #3922 then realized it was closed. I think that this is wrong and should be fixed. My comment below:


I would like to chime in as someone who just had to track this issue down. I think that assuming that anyone who tries to define a "domain" property is using objects as "bag-o-data" stores is a bit off.

In my case, I have a "XyzClient" instance that inherits from EventEmitter. This "XyzClient" reaches into a endpoint, where a set of 5-10 "services" will be made available. In my case one of these services was named "domain". The client then assigned a XyzClient.domain property, for the user to have easy access to this service. Is there a way around this? Sure I can make it XyzClient.foobar('domain') for example. However, at this point how is XyzClient any more wrong then the EventEmitter?

Lets just say I made a bad design decision.. which is subjective but I can accept that, regardless the NodeJS team has to EXPECT bad code to be written at times. I mean javascript is not exactly the paragon platform of best coding practices.

This is the first issue I've needed to track down on github and found a discussion for, but as a new advocate for NodeJS I am a bit disappointed to see a rather elitist position here of "YOUR CODE WRONG" as opposed to looking at the bigger picture and understanding your users will not write code how you think they should.

If you guys wont fix this, I have to think what happens if in version 1.0 of node you guys decide to add a property to EventEmitter or another core class I inherit and depend upon called "CorePublicPropertyOfMyLibraries", in the process of which you break all of my code? Knowing that it's my problem, because well, I shouldn't of added the property to begin with!

Sorry this came a little lengthy, but I just wanted to voice my opinion on the importance of a humble foresight into the horrible things your users will do with your libraries.

@hackedy
Copy link

hackedy commented Apr 16, 2013

I don't think this will be changed--the events api is locked. I mean, there's no mention of domain anywhere on that documentation, so perhaps it will get turned into _domain.

IMO the solution to this problem isn't naming, it's documentation. Write a paragraph in the events.md file about using domain, make it sound less angry than this issue, and put it in a pull request!

@isaacs
Copy link

isaacs commented Apr 17, 2013

@cstockton I can appreciate that you're feeling frustrated because the word "domain" is a bit general, and collides with something you have. For what it's worth, I can assure you that we won't be adding anything else to EventEmitter any time soon, and we do take very seriously the complications in doing so. For example, EventEmitter.listenerCount() was added as a class method rather than a prototype method for exactly this reason.

If it was at all reasonable or performant to use symbol objects or weak maps, I gladly would have done that instead. However, with the current state of V8 JavaScript, that's not a realistic option for code as hot as this path. Only a "normal" object property will do. Since we do need to have a reference from the EE object to the domain object, there's really no way to do that without picking some field to hang it on.

At this point, there's no going back. Please work around using a different field name, or a get function, or some other technique.

@isaacs isaacs closed this as completed Apr 17, 2013
@cstockton
Copy link
Author

I appreciate the empathetic response; and appreciate the reassurance of careful design around these core libraries. I have decided to simply wrap the eventemitter in this specific class instead of inherit directly and proxy the functionality.

Because the first thing I did was go to that page and hit Cntrl + F "domain", I feel there is enough merit to add a small section in regards to this in the Events documentation. I will send a pull request sometime this weekend. Thanks again for the response.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants