Fix onBell listener being registered after term dispose, allow for DisposableStore-managed disposableTimeouts #193583
Conversation
Tyriar
changed the title
| return; | ||
| } | ||
| if ((o as unknown as DisposableStore) === this) { | ||
| throw new Error('Cannot dispose a disposable on itself!'); |
There was a problem hiding this comment.
Does this happen in practice? If so, I think the error message should be adjusted and BugIndicatingError should be used.
There was a problem hiding this comment.
🤷 just doing what add does
There was a problem hiding this comment.
At least the error message is weird.
| * disposable even when the disposable is not part in the store. | ||
| */ | ||
| public delete<T extends IDisposable>(o: T): void { | ||
| if (!o) { |
There was a problem hiding this comment.
Type checks should prevent this from ever happening:
I would either change it to o: T | undefined or remove that check (otherwise it might swallow fishy runtime behavior).
There was a problem hiding this comment.
It aligns with Set.delete, the if (!o) check if to make it work the same as DisposableStore.add
| * Deletes a disposable from store and disposes of it. This will not throw or warn and proceed to dispose the | ||
| * disposable even when the disposable is not part in the store. | ||
| */ | ||
| public delete<T extends IDisposable>(o: T): void { |
There was a problem hiding this comment.
I know it is called Set.delete, but I think this method should be called remove. Better even would be removeAndDispose
There was a problem hiding this comment.
I was going to call it remove, but it's using add and delete internally on Set which aligns with the web api. Instead of delete and deleteAndDispose, I flipped them do delete (and dispose, common case) and deleteAndLeak (rare case).
There was a problem hiding this comment.
I just think it might be surprising that delete also disposes the object.
There was a problem hiding this comment.
I would think the opposite, the main concern is leaks so disposing is the safe option imo.
There was a problem hiding this comment.
We can be explicit on both though, as long as the leak one is called out
Fixes #190251