-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[main] offlineListener preparation #2186
Conversation
function addListener(callback: OfflineCallback) { | ||
listenerList.add(callback); | ||
return { | ||
rm: () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, the list should be an array and an edge case that has to be handled is that someone "could" call this rm
function multiple times, so the first time will remove the entry (if present) and subsequent times will do nothing.
On way to do this is to use a temporary variable and then when rm
is called you just reassign the rm
function on that variable (that you return) to a function that does nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function addListener(callback: OfflineCallback) {
listenerList.push(callback);
const rm = (callback: OfflineCallback) => {
let removed = false;
return () => {
if (!removed) {
const index = listenerList.indexOf(callback);
if (index !== -1) {
// Remove the callback from the list
listenerList.splice(index, 1);
removed = true; // Mark it as removed
}
}
};
};
return rm;
}
does this works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like it does not work, vs is complaining the type error
This reverts commit aa7e0a0.
} | ||
|
||
function setOnlineState (uState: eOfflineValue){ | ||
this.uState = uState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the this
is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any tests for this as any tests should be failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not have any test on this new code yet, maybe we could merge this into karlie's offline branch instead of main branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current test is failing because of dep, I will rerun the rush update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets just push into main
as we are now not planning any more releases from this branch until this feature set is complete.
If we need to release a version 3.0.5 we will create a branch for that release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets wait until we do the patch release before committing this, so that we don't have to branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ready to go
No description provided.