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

Remove EventEmitter#domain clobbering by 'domain' module. #3932

Closed
wants to merge 1 commit into from
Closed

Remove EventEmitter#domain clobbering by 'domain' module. #3932

wants to merge 1 commit into from

Conversation

ashtuchkin
Copy link

Field clobbering is removed on 2 levels:

  • The field itself is renamed to '_domain', according to conventions for internal field naming.
  • The 'events.usingDomains' is checked so that programs that don't use 'domains' module are not affected even if they use '_domain' field.

Fixes #3922.
Test included.

Field clobbering is removed on 2 levels:
 * The field itself is renamed to '_domain', according to conventions for
   internal field naming.
 * The events.usingDomains is checked so that programs that dont use 'domains'
   module are not affected even if they use '_domain' field.

Fixes #3922.
Test included.
@bnoordhuis
Copy link
Member

/cc @isaacs

@ghost ghost assigned isaacs Sep 10, 2012
@isaacs
Copy link

isaacs commented Sep 12, 2012

So, you've just moved the problem from the domain field name to the _domain field name.

Either way, you have the exact same hazard. Except, now, rather than a public documented part of the EE api, it's something private. ("Why are you clobbering the private members of my class!?")

It would make more sense to me to check whether this.domain instanceof Domain perhaps, and avoid setting it if there's already something there. But just moving it from domain to _domain does not improve things.

@isaacs isaacs closed this Sep 12, 2012
@ashtuchkin
Copy link
Author

I agree with that argumentation. The solution I proposed is not clean. One question - are you planning to make domain field official? It's not documented right now as far as I know.

If yes, then I think its a bad decision. It'll create lots of wtfs with existing code without providing lots of usefulness. Arguably, this is a domain module implementation detail and users will rarely need to access or modify this field.

If no, then I'd still suggest making it private, like _events. Maybe even use double underscore to show framework-level private-ness. This is in addition to the instanceof check you suggested.

@bentaber
Copy link

Pull #3998 solves this issue in a different way, using an instanceof check and leaving the .domain interface in tact, baselined off of the v0.8 branch. Just trying to bump the visibility on that one. Thanks.

/cc @isaacs

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

Successfully merging this pull request may close these issues.

4 participants