-
Notifications
You must be signed in to change notification settings - Fork 530
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
gitbutler-feeds #5637
gitbutler-feeds #5637
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
a5f1e81
to
ef72e55
Compare
4a52079
to
8d22894
Compare
8d22894
to
324760a
Compare
3f44693
to
e479dcd
Compare
40f336f
to
924b7f2
Compare
} as Subscription<Arguments>; | ||
} | ||
|
||
this.subscriptions.push(subscription); |
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.
When a previous subscription was found on lines 21:23 this line will push a duplicate. Maybe move this to line 32?
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.
Thanks, this is indeed a subtle problem
|
||
subscription.interval = setInterval(() => { | ||
subscription.lastCalled = Date.now(); | ||
callback(); |
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 interval is meant to be shared by concurrent subscribers right? The callback
argument to this function is ignored unless subscription.counter === 0
.
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.
If the arguments are the same, then the callback should also be the same definition.
That is however implicit and not enforced. I'll think about how to restructure this in such a way that it's certain that we have just one callback per subscription.
lastCalled: number; | ||
} | ||
|
||
export class InterestStore<Arguments> { |
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.
Could we get a description for this class? I'm also curious if there might be a more descriptive name you can give this. Even after becoming familiar with the code I still get a bit confused when I see something.createInterest(...)
.
924b7f2
to
9fe6da2
Compare
This is part 2 of 2 in a stack made with GitButler: