Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

AlexandrHoroshih
Copy link
Member

@AlexandrHoroshih AlexandrHoroshih commented Oct 1, 2021

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

  • Create directory for new method in root in param-case
  • Place code to {method-name}/index.js in CommonJS export in camelCase named export
  • Add types to {method-name}/index.d.ts
  • Add tests to {method-name}/{method-name}.test.ts
  • Add fork tests to {method-name}/{method-name}.fork.test.ts
  • Add type tests to test-typings/{method-name}.ts
    • Use // @ts-expect-error to mark expected type error
    • import { expectType } from 'tsd' to check expected return type
  • Add documentation in {method-name}/reade.md
    • Add header Patronum/MethodName
    • Add section with overloads, if have
    • Add Motivation, Formulae, Arguments and Return sections for each overload
    • Add useful examples in Example section for each overload
  • Add section to README.md in root
    • Add method to table of contents - [MethodName](#methodname)
    • Add section ## [MethodName](/method-name 'Documentation')
    • With summary and simple example

Copy link
Member

@igorkamyshev igorkamyshev left a 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.

README.md Outdated Show resolved Hide resolved
abort/abort.fork.test.ts Outdated Show resolved Hide resolved
abort/index.js Outdated Show resolved Hide resolved
abort/index.d.ts Outdated
params: Params,
config: { onAbort: (callback: () => void) => () => void },
): Done;
getKey(params: Parameters<typeof c['handler']>[0]): UnitValue<typeof c['signal']>;
Copy link
Member

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.

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 2, 2021

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

Copy link
Member

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.

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 4, 2021

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so.

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 11, 2021

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({
Copy link
Member

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 } 🤔

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 2, 2021

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

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 2, 2021

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

Copy link
Member

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.

Copy link
Contributor

@yumauri yumauri Oct 4, 2021

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), () => { ... })

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 13, 2021

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, () => { ... })

Copy link
Member Author

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)

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Nov 3, 2021

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?

Copy link
Member

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?

Copy link
Member

@zerobias zerobias Nov 3, 2021

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/readme.md Outdated Show resolved Hide resolved
abort/readme.md Outdated Show resolved Hide resolved
abort/readme.md Outdated Show resolved Hide resolved
abort/readme.md Outdated Show resolved Hide resolved
@AlexandrHoroshih AlexandrHoroshih marked this pull request as ready for review October 3, 2021 15:09
@sergeysova sergeysova added the feature New functionality label Oct 9, 2021

expect(scope.getState($count)).toEqual(0);
expect(fn).toHaveBeenCalledTimes(3);
expect(fn.mock.calls.every(([error]) => error instanceof AbortedError)).toEqual(
Copy link
Member

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()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this

expect(argumentHistory(fn)).toMatchInlineSnapshot(`
Array [
1,
]
`);

i.e. is not overriden by generic types
@AlexandrHoroshih
Copy link
Member Author

AlexandrHoroshih commented Oct 11, 2021

Updated abort implementation to new patronum approach to operator implementations

Important questions:

  • Abortable Effect now throws special kind of Error, if effect was manually aborted. It would be very conventional to have special helper for failData to filter AbortedError. Discussion: implement abort method #168 (comment)
  • Abortable Effect now checks call keys and signal key by ===. But it might make sense to add special values for common cases (like ALL to abort all in-flight calls) or maybe even custom key-filter function to support multiple ways to group effect calls at once. Discussion: implement abort method #168 (comment)

Feel free to join these discussions

Copy link

@Tauka Tauka left a 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();
Copy link

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

Copy link
Contributor

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

Copy link

@Tauka Tauka Oct 14, 2021

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;
   },
 });

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 14, 2021

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

Copy link
Contributor

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)
});

Copy link

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

Copy link
Member Author

@AlexandrHoroshih AlexandrHoroshih Oct 14, 2021

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

Copy link
Contributor

@yumauri yumauri Oct 15, 2021

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.

@Lonli-Lokli
Copy link

Why don't create abort property on createEffect?

@sergeysova
Copy link
Member

@Lonli-Lokli

const hideFx = createEffect(() => {
  document.body.classList.add('hidden');
})

hideFx.abort();

What the result of .abort() call we need to do inside effect?

@AlexandrHoroshih
Copy link
Member Author

AlexandrHoroshih commented Jan 6, 2023

Closed because the idea needs serious work

Good parts.

  • The onCancel hooks inside the effects body to register cleaning callbacks
  • Call key idea, to cancel only certain calls based on logical rules

Bad parts.
getKey doesn't work if there are no parameters to get the call key/identifier, it's not clear what to do if there are multiple calls with the same call key.
The idea of using the "call group" label to abort all calls from that group sounds good, but it doesn't work if we actually want both options - cancelling the entire group (e.g., absolutely all calls to this effect) in one case, and only certain calls in another.
This suggests that one call should have multiple keys, or maybe there should be some generic abortKeyFilter for effect - but that's not very convenient, since all the logic options for choosing specific calls to cancel are merged into either one keyFilter function or a key generator.

@AlexandrHoroshih AlexandrHoroshih deleted the feat/abort branch March 20, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality needs triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants