Vscode: Stop using EventEmitter

[jrieken]

We have two eventing solutions: The old one and not yet fully adopted one... For real, there are 1044 references to the new Event but still 66 to the old EventEmitter. That means we are almost done and finish this.

This is the output from my reference++ extension, assigning people based on that list

Found 2 references

file:///Users/jrieken/Code/vscode/src/vs/base/common/eventEmitter.ts
  194   * Same as EventEmitter, but guarantees events are delivered in order to each listener
  195   */
  196: export class OrderGuaranteeEventEmitter extends EventEmitter {
  34: export class EventEmitter implements IEventEmitter {
  35
  36    protected _listeners: IListenersMap;

[joaomoreno]

YES

[isidorn]

I will do the Sash and the StartDebugActionItem. I can also do the ActionBar
@jrieken thanks for this

[sandy081]

Removed it in testConfigurationService and workbenchTestServices

[joaomoreno]

@jrieken It would be great to convert this to a checkbox list.

[isidorn]

Done. The removal from the ActoinBar ripled through to a lot of files so it might be worth to regenerate that list.

[rebornix]

EventEmitter removed from editor area (FindState and ReferenceModel).

[rebornix]

I reran the scripts and found that editor modelServiceImpl https://github.com/Microsoft/vscode/blob/master/src/vs/editor/common/services/modelServiceImpl.ts#L10 and viewModelImpl are still relying on EventEmitter.

[alexdima]

The new event emitter infra does not support the following features:

1. in order guaranteed event delivery. i.e.:
   
   
   
   • if an object A produces event e1.
   
   
   • and there are two listeners listener1 and listener2
   
   
   • if listener1 decides to interact again with object A which leads to the producing of event e2.
   
   
   • for certain cases (e.g. buffer and content changes) it is critical that listener2 first receives e1 and only then e2.
   
   
   • as sad as it sounds, I am not making this stuff up, people make edits to models in the change listener, or change decorations in the decorations change listener.
   
   

1. addBulkListener - I think I can rewrite my code to no longer use it; not 100% sure yet.

[jrieken]

@alexandrudima Yeah, did think about in-order delivery yet but I believe it's a good property in general. I have pushed e133265 which optimistically enables that for all Emitters. Tests et all are happy so let's take the risk... If we see issues with it, we can make it an option.

[joaomoreno]

@jrieken I've converted everything to checkboxes...

Btw e133265ba8aee4dc033ba0821b8c12fc4ec58fcb is a brave move. But I'd do the same. 💪

[dbaeumer]

Done for task specific code.

[joaomoreno]

Just did the tree: https://github.com/Microsoft/vscode/commit/6c3c74ab21e463ffc8804d481490df71028beb09

This was an insane refactoring. #GoodLuckDoingThatWithJavaScript #TypeScriptForever

[jrieken]

Updated the description with what's left. @joaomoreno there is still a little in for you... Also unsure who owns contextview.ts?

[isidorn]

I will do the contextview

edit: done. was trivial

[joaomoreno]

Thanks @isidorn!

[joaomoreno]

Did the rest of tree and button. A few todos for @isidorn and @jrieken. It would be great to get a lint error when we forget to do something with the disposable...

[joaomoreno]

Did one last push on the stuff I knew how to. Found quite a few places where we don't collect the disposables from events... fixed what I could and left TODO@[name] markers behind otherwise.

I believe only TextModel is left. It's time for the master to destroy its creation... once and for all! @alexandrudima

Thanks for the push @jrieken!

[isidorn]

@joaomoreno thanks, the only todo I found is in empty view and fixed via 738d907bda

[joaomoreno]