Fixes #163; Created new windowMaid.js #168
Conversation
… more robust window destruction. Destroyed windows can remain in memory, so that needs to be inspected. Also allow clients to remove windows from the cache explicitly fraserxu#163
lib/windowMaid.js
Outdated
| @@ -18,11 +18,11 @@ class WindowMaid { | |||
| * When a job creates a window it invokes this method so any memory leaks | |||
| * due to hung windows are prevented. This can happen if an uncaught | |||
| * exception occurs and job.destroy() is never invoked. | |||
| * @param job | |||
| * @param exportJob the ExportJob instance, it must have a window reference set | |||
|
+1 |
lib/windowMaid.js
Outdated
| // them | ||
| const windowCache = {} | ||
|
|
||
| class WindowMaid { |
There was a problem hiding this comment.
It looks like everything in this class is accessed statically, so you could also set this up as just a simple object. Since the object isn't intended to be instantiated, it may be a bit more self-documenting that way.
var windowMaid = {
registerOpenWindow: function() {}, //etc
}
module.exports = windowMaid
There was a problem hiding this comment.
I've been using Classes in this module. Is there a real benefit to using an object literal instead of the class? Or you just think its a bit more concise?
There was a problem hiding this comment.
I think there are a couple benefits to using an object literal:
- It's more concise, and follows the KISS principle
- It makes the code more self-documenting by clearly indicating the intent. Usually classes are intended to be instantiated. If it's just a simple object, there's no chance anyone can try to instantiate it because that would be impossible.
There was a problem hiding this comment.
I don't see how the static keywords make it any more complex, but I don't mind changing it either so I'll do what you suggest.
And if it was part of a public API I think the argument about instantiation makes great sense. Since it's not in this case I'm not super concerned about that.
|
+1 |
No description provided.