-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EventEmitter instances no longer freezable (As of node 4.0) #2864
Comments
@ryan-rowland Could you explain in more detail why you're use case requires this? I'm not sure if we can ever make guarantees that you can freeze internals. It may be better to use an npm replacement for built-in eventemitters for this: https://www.npmjs.com/search?q=eventemitter |
@Fishrock123 An example case: We have an AccessToken class whose constructor takes a compatible JWT string and parses it into its components, exposing them to the developer (And our own internals). This may contain username, roles, JWT expiration, etc. The token can't be modified once it has been created; it's signed. Once the AccessToken has been created based on the parsed information, it is returned frozen as its values will never change. We're using EventEmitter to emit The desire to freeze the object is intended as self-documentation to convey a strong statement of immutability. From a design perspective, I believe it would be nice to enable this behavior natively however I understand this may not necessarily be a priority. |
Fwiw, origin commit: 7061669 |
@ryan-rowland node 0.14? Did you mean 0.12 or ? |
Perhaps JavaScript isn't the language you want for this .. Sure, we're heading towards a place where you can freeze objects, protect private data with Symbols and the like but this is not JavaScript's forte and you're putting unreasonable expectations on Node core to support such conventions. As @Fishrock123 said, it might be best to pursue an npm installable alternative (there are lots of alternative event emitters in npm) that can provide such guarantees, the core documentation makes no such claims and it's never been within the scope of EventEmitter to do this and I'm pretty sure we don't want this to be within scope because of the potential performance implications and constraints on the kinds of optimisations we may want to apply (and have already applied). |
@mscdex Whoops, typo no doubt brought on by thinking "4.0" while typing "1.2". 😀 Thanks! @rvagg This is a client-side SDK for in-browser JS that is being compiled via Browserify; JavaScript is definitely the language we want for this. It is true and I agree that JS modules commonly don't see much in the way of object protection, but one of our core values of creating an SDK for a customer developer is that it should be as intuitable and infallible as possible. Freezing such objects implicitly explains something about its use, and also guides the customer developer away from bad practices. I haven't researched, in any depth, the optimizations you've made other than the changes linked to in the commit above. At the end of the day, this is simply a nice-to-have and we can always enforce it ourselves. I understand there are higher priority considerations that may conflict. Thanks for your time. |
In our use-case, we're surfacing immutable objects that inherit from EventEmitter to the developers using our SDKs. As of node 0.12 this behavior was valid however as of 4.0 we get errors when EventEmitter attempts to modify the
_eventsCount
property on the instance, which is read-only because the object has been frozen.In #1817 it's made clear that
_events
may already be getting moved to a getter/setter deal. If so, implementing_eventsCount
through getter/setter would allow the inheriting instance to be frozen.Alternatively, the
events
data could hang off of the EventEmitter instance in its own special hash, ie:The text was updated successfully, but these errors were encountered: