feat: allow to register subscriptions by key #1529
Conversation
MartinCupela
requested review from
vishalnarkhede,
santhoshvai,
khushal87,
szuperaz,
oliverlaz,
arnautov-anton and
myandrienko
as code owners
|
Size Change: +636 B (+0.17%) Total Size: 379 kB
|
| private subscribeMessageComposerConfigStateChanged = () => | ||
| this.configState.subscribeWithSelector( | ||
| ({ text }) => [text] as const, | ||
| ([currentText]) => { | ||
| if (this.textComposer.text === '' && currentText.defaultValue) { | ||
| this.textComposer.insertText({ | ||
| text: currentText.defaultValue, | ||
| selection: { start: 0, end: 0 }, | ||
| }); | ||
| } | ||
| }, | ||
| ); | ||
| this.configState.subscribe((nextValue, previousValue) => { | ||
| const { text } = nextValue; | ||
| if (this.textComposer.text === '' && text.defaultValue) { | ||
| this.textComposer.insertText({ | ||
| text: text.defaultValue, | ||
| selection: { start: 0, end: 0 }, | ||
| }); | ||
| } | ||
|
|
||
| if (!previousValue?.drafts.enabled && nextValue.drafts.enabled) { | ||
| this.addUnsubscribeFunction(this.subscribeDraftUpdated(), 'draft.updated'); | ||
| this.addUnsubscribeFunction(this.subscribeDraftDeleted(), 'draft.deleted'); | ||
| } | ||
| if (previousValue?.drafts.enabled && !nextValue.drafts.enabled) { | ||
| this.unregisterSubscriptionsByKey('draft.updated'); | ||
| this.unregisterSubscriptionsByKey('draft.deleted'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I understand what you're trying to say but I'm not sure if I like this approach. State-dependent subscriptions can be resolved using subscribe method's scope (I've used similar approach here):
private subscribeMessageComposerConfigStateChanged = () => {
let draftUnsubscribeFunctions: Unsubscribe[] | null;
const unsubscribe = this.configState.subscribeWithSelector(
(currentValue) => ({
textDefaultValue: currentValue.text.defaultValue,
draftsEnabled: currentValue.drafts.enabled,
}),
({ textDefaultValue, draftsEnabled }) => {
if (this.textComposer.text === '' && textDefaultValue) {
this.textComposer.insertText({
text: textDefaultValue,
selection: { start: 0, end: 0 },
});
}
if (draftsEnabled && !draftUnsubscribeFunctions) {
draftUnsubscribeFunctions = [
this.subscribeDraftUpdated(),
this.subscribeDraftDeleted(),
];
} else if (!draftsEnabled && draftUnsubscribeFunctions) {
draftUnsubscribeFunctions.forEach((fn) => fn());
draftUnsubscribeFunctions = null;
}
},
);
return () => {
draftUnsubscribeFunctions?.forEach((unsubscribe) => unsubscribe());
unsubscribe();
};
};With key-based approach (at least in this form) we're limited to one unsubscribe function per key, if we decide to make it a Map<string, Set<Unsubscribe>> (to allow registering more of the same subscription) then we're introducing another unnecessary complexity IMO. I'd always hold onto the functions we get from registering subscriptions as true "keys" per-se and utilize function/method scopes as much as possible.
There was a problem hiding this comment.
One extra - say if we had subscribeWhatever(someInput: unknown) and your registerSubscriptions would look something like this:
public registerSubscriptions = () => {
if (this.unsubscribeFunctions.size) {
return noop;
}
this.unsubscribeFunctions.add(this.subscribeWhatever(1));
this.unsubscribeFunctions.add(this.subscribeWhatever(2));
}Then... if we went key-based route, you'd have to make sure this someInput (of whatever type) would have to be considered when you're constructing the key otherwise you could overwrite the unsubscribe function it holds (in its simple form).
With scopes you don't even have to think about this extra overhead.
There was a problem hiding this comment.
Understood, it all makes sense. Let's go the scope way. Do you plan to introduce it in your PR?
There was a problem hiding this comment.
I'll cherry-pick your changes and adjust, thanks. 👍
Suggestions regarding comments: