ref(core): Improve promise buffer #17788
Conversation
| * @returns Removed promise. | ||
| */ | ||
| function remove(task: PromiseLike<T>): PromiseLike<T | void> { | ||
| return buffer.splice(buffer.indexOf(task), 1)[0] || Promise.resolve(undefined); |
There was a problem hiding this comment.
I don't think it is necessary to wait on anything here, we can just remove this from the buffer directly and be done with it - we already execute the function anyhow without this.
mydea
force-pushed
the
fn/better-promise-buffer
branch
from
bed62c2 to
f1aedb6
Compare
| expect(buffer.add(producer1)).toEqual(task1); | ||
| void expect(buffer.add(producer2)).rejects.toThrowError(); | ||
| // This is immediately executed and removed again from the buffer | ||
| expect(buffer.$.length).toEqual(0); |
There was a problem hiding this comment.
You can see the slight change of timing semantics here, in that the promise is immediately added & removed from the buffer if it is a sync promise. This makes sense IMHO and is possibly a tiny micro-optimization, I suppose.
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
mydea
force-pushed
the
fn/better-promise-buffer
branch
from
d737fed to
d8d6dec
Compare
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mydea
force-pushed
the
fn/better-promise-buffer
branch
from
d8d6dec to
be6b08e
Compare
| expect(buffer.add(producer)).toEqual(promise); | ||
| expect(buffer.add(producer2)).toEqual(promise); | ||
|
|
||
| expect(buffer.$.length).toEqual(1); |
There was a problem hiding this comment.
Do you think this is a case we could even run into in real life? (don't think this is wrong per sé just a bit interesting because we handle this differently e.g. in client hook subscribers)
There was a problem hiding this comment.
yeah not quite sure, I kept this behavior (this was already that way) the same as it was before - honestly I think it is not really "desired"/"needed" but just a way to make sure we can easily remove the promises from the buffer again 😅
Extracted this out of #17782, this improved our promise buffer class a bit:
limit, as we never use this (there is a default limit used). There is also really no reason to use this without a limit, the limit is the whole purpose of this class.Setinstead of an array for the internal buffer handling, this should slightly streamline stuff.drain, we can simplify the implementation without a timeout drastically. We can usePromise.race()to handle this more gracefully, which should be supported everywhere.