-
Notifications
You must be signed in to change notification settings - Fork 10
Conversation
❤️ |
cc0e911
to
6ff6a76
Compare
❤️ |
helpers.ts
Outdated
@@ -337,6 +338,16 @@ export function isPromise(candidateFuture: any): boolean { | |||
return !!(candidateFuture && typeof (candidateFuture.then) === "function"); | |||
} | |||
|
|||
export async function attachAwaitDetach(eventName: string, ee: EventEmitter, cb: Function, operation: Promise<any>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for readability I wouldn't name the variables like this in an exported function - ee/cb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
helpers.ts
Outdated
@@ -337,6 +338,16 @@ export function isPromise(candidateFuture: any): boolean { | |||
return !!(candidateFuture && typeof (candidateFuture.then) === "function"); | |||
} | |||
|
|||
export async function attachAwaitDetach(eventName: string, ee: EventEmitter, cb: Function, operation: Promise<any>) { | |||
ee.on(eventName, cb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering, this looks a lot like the using(x: IDisposable) { }
pattern in C# and try-with-resources
pattern in Java, do you think it would be appropriate for us to add a Utility method which can handle it following the same pattern?
I imagine usage be like:
using(new DisposableEventsWrapper(eventName, eventsCallback), (platformData.projectRoot, projectData, buildConfig) => { await platformData.platformProjectService.buildProject(platformData.projectRoot, projectData, buildConfig)) }
The purpose would be to be more-readable and reusable? We'll have to add IDisposable/using patterns and implement a DisposableEventsWrapper for generic event handling.
What do you think? It can be perceived as an over-engineering in the current case?
I've found something similar here: https://gist.github.com/dsherret/cf5d6bec3d0f791cef00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be a bit of an overkill for this particular task. I really do like the idea though, in fact I've considered something similar in the past but never got around to implementing it.
A concern of mine is the fact that we already have IDisposable
interface and a mechanism in our dependency injection for calling .dispose()
. However you certainly wouldn't want to call .dispose()
on the classes that currently implement IDisposable
yourself. That would create a bit of confusion IMO.
❤️ |
Its purpose is to attach an event handler, await an action and detach the event handler
1e35769
to
fb05ce7
Compare
Ping @yyosifov with fixed comments. Had to push force the branch because of a minor merge conflict - sorry for that |
❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if passing the operation as a Promise or as a method that returns Promise is better, but as I do not have a strong opinion on this, I approve current changes.
Its purpose is to attach an event handler, await an action and detach the event handler
Ping @rosen-vladimirov @TsvetanMilanov