-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
implement abort
method
#168
Conversation
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.
Great job! 💙
I've left some comments. But all of them are just opinions.
abort/index.d.ts
Outdated
params: Params, | ||
config: { onAbort: (callback: () => void) => () => void }, | ||
): Done; | ||
getKey(params: Parameters<typeof c['handler']>[0]): UnitValue<typeof c['signal']>; |
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 make getKey
optional?
In my mind, it's a popular situation, when you have to cancel all in flight effects and starts new one.
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.
I'm not sure about that 🤔
What kind of key should be default for that?
Maybe it is better to make getKey
required for now and add such defaults only after real usage feedback?
Also, i was thinking of something like special values, that can be used for common cases 🤔
import {
ALL,
abort
} from 'patronum/abort'
cancel(ALL) // will cancel all calls in flight, no matter what kind of keys were used for each call
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.
I think, () => ALL
(Symbol
, maybe?) is a good default for the getKey
.
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.
Should this ALL value be available, even if other getKey option is set?
Like this
abort({
signal,
handler,
getKey: p => p.id
})
signal(42)
// can abort effect call by id
signal(ALL)
// also still can abort all, by using magic value
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.
I think so.
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.
What about custom key filter function?
Like this:
const ALL = "all"
abort({
signal,
handler,
getKey: p => p.id > 7 ? `prefix/${p.id}` : p.id,
keyFilter: (param, key) => {
// custom case - magic value
if (param === ALL) return true;
// custom case - specific prefix for a group of keys
if (param.includes("prefix")) return true;
// basic case - just `===` identity
return param === key;
}
})
signal(42)
// can abort effect call by id, basic case
signal(ALL)
// also still can abort all, by using magic value, custom case
signal("prefix")
// cancel all values that fit in specific group, custom case
I think, this is a best option, but not sure, if it even possible to type this in typescript to keep good type inference 🤔
abort/readme.md
Outdated
}, | ||
}); | ||
|
||
guard({ |
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.
Maybe it's better to return from abort
an object { abortableFx, abortedEvent }
🤔
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.
I'm not sure about that 🤔
User still would have to manually filter all fail
and failData
calls 🤷
Especially, if abortable effect is used through attach
or called inside some other effect
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.
Maybe something like this would do?
import { abort, deriveAborted } from "patronum/abort"
const fx = abort({ ... })
const {
aborted, // filtered fx.fail, only with AbortedError
failed // filtered fx.fail, only if not AbortedError
} = deriveAborted(fx)
// Can take any effect
// Useful, if abortable effect is somewhere deep in chain of `attach`-ed effects
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.
Yeah, I like this idea.
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.
This looks weird IMO.
What about wrappers?
import { abort, aborted, failed, abortedData, failedData } from "patronum/abort"
const fx = abort({ ... })
const store = createStore(null)
.on(failed(fx.fail), () => { ... })
.on(aborted(fx.fail), () => { ... })
.on(failedData(fx.failData), () => { ... })
.on(abortedData(fx.failData), () => { ... })
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.
So i think, in the end, question is which option looks better?
1. Derive all possibly needed events from effect, keep it in one container object
import { abort, deriveAborted } from "patronum/abort"
const cancelRequest = createEvent()
const baseRequestFx = abort({ ... })
const requestFx = attach({
source: $tokenOrSomething,
mapParams: ...,
effect: baseRequestFx,
})
// Can take any effect
// Useful, if abortable effect is somewhere deep in chain of `attach`-ed effects
const requestFxEvents = deriveAborted(requestFx)
// then use normally
requestFxEvents.aborted.watch(console.log) // filtered fx.fail, only with AbortedError
requestFxEvents.failed.watch(console.log) // filtered fx.fail, only if not AbortedError
patronum.debug(requestFxEvents.failedData, requestFxEvents.abortedData)
const store = createStore(null)
.on(requestFxEvents.failed, () => { ... })
.on(requestFxEvents.aborted, () => { ... })
.on(requestFxEvents.failedData, () => { ... })
.on(requestFxEvents.abortedData, () => { ... })
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.
2. Custom helpers for each case
import { abort, aborted, failed, abortedData, failedData } from "patronum/abort"
const cancelRequest = createEvent()
const baseRequestFx = abort({ ... })
const requestFx = attach({
source: $tokenOrSomething,
mapParams: ...,
effect: baseRequestFx,
})
// Can take any effect or event
const store = createStore(null)
.on(failed(requestFx.fail), () => { ... })
.on(aborted(requestFx.fail), () => { ... })
aborted(requestFx.fail).watch(console.log) // filtered fx.fail, only with AbortedError
failed(requestFx.fail).watch(console.log) // filtered fx.fail, only if not AbortedError
patronum.debug(failedData(requestFx.failData), abortedData(requestFx.failData))
// or
const requestFailed = failed(requestFx.fail);
const requestAborted = aborted(requestFx.fail)
const requestFailedData = failedData(requestFx.failData)
const requestAbortedData = abortedData(requestFx.failData)
const store = createStore(null)
.on(requestFailed, () => { ... })
.on(requestAborted, () => { ... })
patronum.debug(requestFailedData, requestAbortedData)
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.
@zerobias what do you think?
Considering that in the future the effector
may also get the abortable effects feature at the core (and, i guess, migration from the patronum
variant should be as simple as possible), which option is better?
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.
@AlexandrHoroshih separate methods which may be applied to arbitrary effects going to be an ambiguous approach and ability of the cancellation to pass through nested effects still does not seem obvious. If you remove a requirement to derive abort triggers from existed effects and support for nesting then api for that triggers may fit configuration object
const timeoutFx = abort({
on: {
abort: [timeoutAborted],
done: [timeoutCompleted],
fail: [setTimeoutNotSupported]
},
handler(params, {onAbort}) {
}
})
And real-world case with timeouts rise another important question: how we would derive id from params if TimeoutID
could be given only during execution at first place? Artificial ids are not an option. In other words, how application should communicate with body of the effect?
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.
You need your implementation to pass reality-check, even a link to repl with code which do something meaningful would be enough to found certain dx issues
abort/abort.fork.test.ts
Outdated
|
||
expect(scope.getState($count)).toEqual(0); | ||
expect(fn).toHaveBeenCalledTimes(3); | ||
expect(fn.mock.calls.every(([error]) => error instanceof AbortedError)).toEqual( |
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.
You can use there argumentHistory
from ../../test-library
and .toMatchInlineSnapshot()
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.
Like this
patronum/src/delay/delay.test.ts
Lines 31 to 35 in bd08c91
expect(argumentHistory(fn)).toMatchInlineSnapshot(` | |
Array [ | |
1, | |
] | |
`); |
98084a6
to
98fda58
Compare
i.e. is not overriden by generic types
Updated Important questions:
Feel free to join these discussions |
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 for that method! It's something i've been waiting for. Here is just a question about implementation
signal: cancel, | ||
getKey: (p: number) => p, | ||
async handler(todoId: number, { onAbort }) { | ||
const controller = new AbortController(); |
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.
Is it possible for abort
to create AbortController implicitly?
async handler(todoId: number, { signal }) {
const response = await fetch(
`https://jsonplaceholder.typicode.com/todos/${todoId}`,
{ signal }
);
const result = await response.json();
return result;
}
So that we don't have to do it ourselves, it seems like common thing to do
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.
This could be easily done, just adding default aborter handler
const controller = new AbortController();
const aborter: { handlers: (() => void)[] } = { handlers: [
() => controller.abort()
] };
// ---8<---
handler(params as any, { onAbort, signal: controller.signal })
But I'm not sure about necessity...
First, AbortController will be created even when it is not required, and second — AbortController is not supported on Node <15
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.
Then how about adding flag, so that we won't create unnecessary AbortControllers? I'm not insisting though, just some thoughts
Personally, cancelling requests is my main use case
const abortableFx = abort({
signal: cancel,
getKey: (p: number) => p,
withAbortController: true,
async handler(todoId: number, { onAbort, signal }) {
const response = await fetch(
`https://jsonplaceholder.typicode.com/todos/${todoId}`,
{ signal }
);
const result = await response.json();
return result;
},
});
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, i don't think it is very convenient, since this operator aims to be a more common option for abortable effects, rather then just requests
For e.g. for some sort of granular animation control you will work with timeouts and intervals and other Web and Node Api's probably can have their own ways to abort some operations
Also, AbortController is not the only option to abort a request, for e.g. axios have its own CancelToken
api and older XMLHttpRequest
api have an .abort()
method + yeah, it is not widely supported at Node yet
So, AbortController is not really universal solution for abortable effects yet
Also, i guess, if we building an api layer with effector effects, this will be defined somewhere in one place anyway, as a base fabric for request effects, like
const createRequest = (config) =>
abort({
signal: config.abortTrigger,
getKey: config.getRequestId,
async handler(params, { onAbort }) {
const controller = new AbortController();
onAbort(() => {
controller.abort();
});
const response = await fetch(config.url(params), {
signal: controller.signal,
});
const result = await response.json();
return result;
},
});
If you looking for more requests-oriented solution, you might want to look at fry-fx
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.
AbortController is not limited to requests only, and I think it will become de-facto standard for any abortable operations, as soon as Node 14 is unsupported
signal.addEventListener('abort', () => {
clearTimeout(timeoutId)
});
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.
Fair enough 🙂 It's probably good idea to keep abort
unopinionated about abort mechanism
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.
AbortController is not limited to requests only, and I think it will become de-facto standard for any abortable operations, as soon as Node 14 is unsupported
According to https://nodejs.org/en/about/releases/ Node 14 will be supported up until 2023, so, maybe, then it will actually be more convinient 🤔
I dont think its needed to be built-in now, though
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.
Yep
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.
TIL that timeouts in Node supports aborting, using AbortController :)
https://www.nearform.com/blog/using-abortsignal-in-node-js/
The AbortController and AbortSignal APIs are quickly becoming the standard mechanism for cancelling asynchronous operations in the Node.js core API.
But I agree, that this is unnecessary for now, in abort
method.
Why don't create abort property on createEffect? |
const hideFx = createEffect(() => {
document.body.classList.add('hidden');
})
hideFx.abort(); What the result of |
Closed because the idea needs serious work Good parts.
Bad parts. |
closes #154
Description
This method creates abortable effects, which is mostly useful for cancellable api calls and all kinds controllable timeouts and intervals (e.g. for granular control over animations)
Checklist for a new method
param-case
{method-name}/index.js
in CommonJS export incamelCase
named export{method-name}/index.d.ts
{method-name}/{method-name}.test.ts
{method-name}/{method-name}.fork.test.ts
test-typings/{method-name}.ts
// @ts-expect-error
to mark expected type errorimport { expectType } from 'tsd'
to check expected return type{method-name}/reade.md
Patronum/MethodName
Motivation
,Formulae
,Arguments
andReturn
sections for each overloadExample
section for each overloadREADME.md
in root- [MethodName](#methodname)
## [MethodName](/method-name 'Documentation')