@sandy081 problematic in settings/keybindings: KeybindingsEditorModel.resolve() and PreferencesService.resolve()

Potentially dangerous (depends on usage): IExtensionService.getExtensions() and IExtensionService.readExtensionPointContributions()

And then a lot in IModeService: getOrCreateMode, getOrCreateModeByLanguageName, getOrCreateModeByFilenameOrFirstLine, et al

Thoughts?

We should remove IExtensionService#onReady. Maybe make it a lifecycle event but surely never block on it but treat it like an event, e.g. to update something

@jrieken yeah I had that thought as well, if we make it as an event it reduces the chance that someone would chain that promise up all the way to some critical code path.

However, if we have an event, that would be an argument against adding it to the lifecycle service, because there we typically have promises.

Problem I see is with the mode service things blocking on the extensions to be ready. Maybe @alexandrudima could chime in if that could also be event based and not blocking.

Fixed it in Settings editor by removing the dependency on IExtensionService#onReady instead listening to default configuration changes - #38329

Re KeybindingsEditorModel.resolve(), Keybindings editor renders Keybindings and unbound commands. There is already an event that is triggered when keybindings are changed / registered. But not sure there is an event triggered when commands change / registered so that the editor can be updated.

@bpasero IModeService.getOrCreateMode() blocks on the extensions ready (i.e. extensions scanned and extension points invoked, not necessarily extension host process ready), but that is IMHO inconsequential as it does not block the creation/displaying of a buffer.

@sandy081 well, the idea is to not join your promise on the extensions ready promise. So the event that you are looking for is still the same ("extensions ready") but you should simply update the editor when it happens instead of waiting for it before the editor is declaring to be loaded.

...I think all of this will be much easier to not do wrong once we replace the promise to be an event.

There is already an event that is triggered when keybindings are changed / registered.

Really, where? I'd copy from it

But not sure there is an event triggered when commands change / registered so that the editor can be updated.

Doesn't exist but can be added

@jrieken here

the idea is to not join your promise on the extensions ready promise

@bpasero Not sure what you mean here. Settings editor is no longer depending on the extensions ready promise. Also it does not wait for any. It simply updates the editor with newly registered default settings by listening to the configuration change event. More details are here #38329.

I would like to do the same for Keybindings editor. But I am missing an event when commands are registered.

@sandy081 makes sense. I was just saying that as a workaround for now you could still react to the extensions loading promise. But in general if we ever want to support installing an extension without window reload then this would not be a good approach, so an event that is specific is always better 👍

Agreed. We just have specific events rather than general extensions loading promise.

@sandy081 could you introduce a new event from IExtensionService that signals extensions have changed? This event could then also be sent when we enable/disable extensions during the session without restart and adopters would be aware that this can happen anytime (like root folders can change anytime even after startup). I see in many cases the current promise is used like an event already.

Sure

@sandy081 I still think we should then maybe go and check for each consumer if a more fine grained event would make sense and would be possible. That could come as a second step.

Things pushed:

• extension host created in running phase (previously restored)
• renamed onReady to whenInstalledExtensionsRegistered
• introduced a new event onDidRegisterExtensions which will fire after the initial extensions are registered as well when we ever enable to enable extensions during runtime without window reload

I was able to replace whenInstalledExtensionsRegistered with the event in some places but had to leave it in for other places. When we approach the topic of allowing to enable extensions without window reload we need to basically revisit each place of whenInstalledExtensionsRegistered and adopt the event as well.

I did a check of all remaining users of whenInstalledExtensionsRegistered and verified that none would block the startup, so I think we should be OK.