Consider adopting the Disposable pattern in inject functions

[chrismwendt]

The root cause of sourcegraph/browser-extensions#178 was that no marker was checked before mounting the command palette. If that function was supposed to return a Disposable, there's a higher chance that teardown logic would have been implemented.

This code looks pretty fragile, and could be made more robust with Disposables

https://github.com/sourcegraph/browser-extensions/blob/ff75251654fdbb7d611d3488d7fef6ccc6d6bcab/src/libs/github/inject.tsx#L96-L112

[felixfbecker]

How exactly would that code become more robust with Disposables?
I believe if the trigger for side effects are events, it's best modelled with Observables, which have cleanup built-in (Subscription is basically a Disposable).

But in general I would be careful with the Disposable pattern. I wrote up some thoughts in an earlier review:

Another general thing that I think we should strive for is to avoid the design of "disposable"/"unsubscribable" classes. This is common in C#, where you also have supporting language features like using statements and finalisers, but JS doesn't have these. A disposable class means any party can get an object into an invalid state by calling dispose on it. The contract of "the constructor brings the object into a usable state, and it is valid till its death (garbage collection)" is no longer true. Objects that were disposed must not be used anymore, but that cannot be enforced if you handed references out. There must be a clear "ownership" of objects, who is responsible for disposing it, but that ownership is often not clear at all when you have object referring to each other, keeping maps, etc.
I'm not saying it doesn't have its places, but we should strive to avoid it where it can be avoided.
In general, it helps to

• Think of objects more as dumb data (like immutable structs) without methods (this usually means prefer object literals over classes)
• Make operations within functions
• Avoid mutation where possible. Especially through the API, the fewer mutable object states we need to track across threads, the better. Functional APIs, one-shot operation APIs are better than mutable objects/handles for those. Again, won't always be possible since the UI is inherently mutable, but we should avoid it where not needed.

sourcegraph/sourcegraph-extension-api#56 (review)

[chrismwendt]

The code that creates the elements could return a function () => void, which, when called, would delete the elements it created. That way the creation and deletion live closer together and are more likely to stay in sync.

It sounds like Subscription could work just as well (aren't they both () => void?).

[stale]

Please post an update or close this with an explanation if it is no longer relevant.
This will be closed automatically if there is no more activity.

[stale]

Stalebot is closing this due to inactivity.