Add signal option and remove abort() method
#53
Conversation
index.js
Outdated
| @@ -91,15 +84,16 @@ export default function pThrottle({limit, interval, strict, onDelay}) { | |||
| }); | |||
| }; | |||
|
|
|||
| throttled.abort = () => { | |||
| signal?.throwIfAborted(); | |||
| signal?.addEventListener('abort', () => { | |||
There was a problem hiding this comment.
You need to remove this listener when the function is done, so not to create event listener leaks.
There was a problem hiding this comment.
It seem to have no way to know that the function is done?
|
#53 (comment) is not done |
index.js
Outdated
| queue.get(timeout)(new AbortError()); | ||
| } | ||
| signal?.throwIfAborted(); | ||
| if (signal && !signal.aborted) { |
There was a problem hiding this comment.
The aborted check is moot as throwIfAborted already takes care of that.
|
I think you could use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry to unsubscribe when the |
index.js
Outdated
| const registry = new FinalizationRegistry(() => { | ||
| signal.removeEventListener('abort', aborted); | ||
| }); | ||
| registry.register(signal, 'signal'); |
There was a problem hiding this comment.
This is incorrect. It's the throttled function that needs to be the target.
And the registry should be defined at the top level:
const registry = new FinalizationRegistry(({signal, aborted}) => {
signal.removeEventListener('abort', aborted);
});There was a problem hiding this comment.
Please review the latest commit, it's ok ?
index.js
Outdated
| @@ -67,6 +60,10 @@ export default function pThrottle({limit, interval, strict, onDelay}) { | |||
|
|
|||
| const getDelay = strict ? strictDelay : windowedDelay; | |||
|
|
|||
| const registry = new FinalizationRegistry(({signal, aborted}) => { | |||
| signal?.removeEventListener('abort', aborted); | |||
| }); | |||
|
You need to fix the merge conflict. |
sindresorhus
changed the title
signal option and remove abort() method
Fixes #51