Improvement/add context to async options #247

MrHus · 2020-01-21T10:30:02Z

Description

The promiseFn and the deferFn have been unified. They now share the following signature:

export type AsyncFn<T, C> = (
  context: C | undefined,
  props: AsyncProps<T, C>,
  controller: AbortController
) => Promise<T>

Before the deferFn and promiseFn had this signature:

export type PromiseFn<T> = (props: AsyncProps<T>, controller: AbortController) => Promise<T>

export type DeferFn<T> = (
  args: any[],
  props: AsyncProps<T>,
  controller: AbortController
) => Promise<T>

The difference is the idea of having a context, the context will contain all parameters
to AsyncProps which are not native to the AsyncProps. For example:

useAsync({ promiseFn: loadPlayer, playerId: 1 })

In the above example the context would be {playerId: 1}.

This means that you know need to expect three parameter for the promiseFn instead of two.

So before in < 10.0.0 you would do this:

import { useAsync } from "react-async"

// Here loadPlayer has only two arguments
const loadPlayer = async (options, controller) => {
  const res = await fetch(`/api/players/${options.playerId}`, { signal: controller.signal })
  if (!res.ok) throw new Error(res.statusText)
  return res.json()
}

const MyComponent = () => {
  const { data, error, isPending } = useAsync({ promiseFn: loadPlayer, playerId: 1 })
}

In 11.0.0 you need to account for the three parameters:

import { useAsync } from "react-async"

// With two arguments:
const loadPlayer = async (context, options, controller) => {
  const res = await fetch(`/api/players/${context.playerId}`, { signal: controller.signal })
  if (!res.ok) throw new Error(res.statusText)
  return res.json()
}

const MyComponent = () => {
  // You can either pass arguments by adding extra keys to the AsyncProps
  const { data, error, isPending } = useAsync({ promiseFn: loadPlayer, playerId: 1 })

  // Or you can explicitly define the context which is TypeScript friendly
  const { data, error, isPending } = useAsync({ promiseFn: loadPlayer, context: { playerId: 1 } })
}

For the deferFn this means no longer expecting an array of arguments but instead a singular argument.
The run now accepts only one argument which is a singular value. All other arguments to run but
the first will be ignored.

So before in < 10.0.0 you would do this:

import Async from "react-async"

const getAttendance = () =>
  fetch("/attendance").then(
    () => true,
    () => false
  )
const updateAttendance = ([attend, userId]) =>
  fetch(`/attendance/${userId}`, { method: attend ? "POST" : "DELETE" }).then(
    () => attend,
    () => !attend
  )

const userId = 42

const AttendanceToggle = () => (
  <Async promiseFn={getAttendance} deferFn={updateAttendance}>
    {({ isPending, data: isAttending, run, setData }) => (
      <Toggle
        on={isAttending}
        onClick={() => {
          run(!isAttending, userId)
        }}
        disabled={isPending}
      />
    )}
  </Async>
)

In 11.0.0 you need to account for for the parameters not being an array:

import Async from "react-async"

const getAttendance = () =>
  fetch("/attendance").then(
    () => true,
    () => false
  )
const updateAttendance = ({ attend, userId }) =>
  fetch(`/attendance/${userId}`, { method: attend ? "POST" : "DELETE" }).then(
    () => attend,
    () => !attend
  )

const userId = 42

const AttendanceToggle = () => (
  <Async promiseFn={getAttendance} deferFn={updateAttendance}>
    {({ isPending, data: isAttending, run, setData }) => (
      <Toggle
        on={isAttending}
        onClick={() => {
          run({ attend: isAttending, userId })
        }}
        disabled={isPending}
      />
    )}
  </Async>
)

Breaking changes

Yes

Checklist

Make sure you check all the boxes. You can omit items that are not applicable.

Implementation for both <Async> and useAsync()
Added / updated the unit tests
Added / updated the documentation
Updated the PropTypes
Updated the TypeScript type definitions

codesandbox-ci · 2020-01-21T10:32:21Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2a45ae2:

Sandbox	Source
quiet-cloud-hkxiw	Configuration

ghengeveld · 2020-01-21T10:32:43Z

.gitignore

@@ -15,3 +15,5 @@ lerna-debug.log*
 # when working with contributors
 package-lock.json
 yarn.lock
+
+.vscode


This should probably be in your global gitignore too!

I've added it because the project explicitly instructs you to create settings for .vscode in the CONTRIBUTING.md.

Yeah I agree we should add it. Just wanted to let you know you can use a global gitignore file for it too.

ghengeveld · 2020-01-21T10:55:13Z

Impressive work! I'm going to need to find some time to take a closer look at this. Very nice to see you updated all the docs too ❤️

freeman · 2020-01-23T14:27:05Z

packages/react-async/src/context.ts

+  KEYS_OF_FETCHOPTIONS.forEach(k => {
+    delete context[k]
+  })


It seems this means it is impossible to have keys in context that conflict with AsyncOptions ...

Shouldn't it delete entries from options but not from options.context ?

Given that this PR is breaking change maybe it should only support passing arguments through options.context to bypass this processing ?

+1 for only passing arguments through options.context. It'll be a pretty big change but for good reason. Still having the option to also use rest props on AsyncProps only complicates things.

I would also like to see the context arg on run to be merged on top of options.context.

@ghengeveld so just to be sure you want the api to change from:

<Async promiseFn={loadPlayer} playerId={1} suspense={true}> useAsync(loadPlayer, { playerId: 1, suspense: true}); // Alt useAsync({ promiseFn: loadPlayer, playerId: 1, suspense: true});

To:

<Async promiseFn={loadPlayer} context={{ playerId: 1}} suspense={true}> useAsync(loadPlayer, { context: { playerId: 1}, suspense: true}); // Alt useAsync({ promiseFn: loadPlayer, context: { playerId: 1}, suspense: true});

I think this would be a great change.

I'm also for making the context pure, as in nothing gets magically added to it or deleted from it. This will make the code more readable.

But I'd also recommend overloading useAsync in the future to allow:

useAsync(loadPlayer, { playerId: 1}, {suspense: true});

As I think this would result in the most readable api.

If we are changing APIs here anyways I'm all for moving from positional parameters to named parameters (one parameter with a keyed object).

So I'd prefer to be only left with

<Async promiseFn={loadPlayer} context={{ playerId: 1}} suspense={true}> useAsync({ promiseFn: loadPlayer, context: { playerId: 1}, suspense: true});

as that will make it more readable at a glance, allow for easier future expansion and will keep the typings more readable/less error-prone.

I'm in favor of dropping the overloaded signatures and only support the single object argument. The shorthand where you pass in the async function as first argument was added to support some very basic use cases where it'd be the only argument, but it's not worth the extra complexity in code and API consistency.

By the way we're going to have to write a codemod for this, because it would otherwise require quite a bit of manual work to upgrade.

I'm also for making the context pure, as in nothing gets magically added to it or deleted from it. This will make the code more readable.

Agreed. People can always spread it themselves. This means deferFn will always only receive the context from run, and the context from options is only passed to promiseFn.

I've updated the code so:

There is no more overload of useAsync.

There is no more magical context spreading / building.

codecov · 2020-01-27T09:14:16Z

Codecov Report

Merging #247 into next will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next     #247      +/-   ##
==========================================
+ Coverage   98.58%   98.59%   +<.01%     
==========================================
  Files           8        8              
  Lines         424      427       +3     
  Branches      148      148              
==========================================
+ Hits          418      421       +3     
  Misses          6        6

Impacted Files	Coverage Δ
packages/react-async/src/propTypes.ts	`100% <ø> (ø)`	⬆️
packages/react-async/src/useAsync.tsx	`99.24% <100%> (+0.01%)`	⬆️
packages/react-async/src/Async.tsx	`100% <100%> (ø)`	⬆️
packages/react-async/src/reducer.ts	`94.11% <100%> (ø)`	⬆️
packages/react-async/src/status.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2664df0...2a45ae2. Read the comment docs.

The `promiseFn` and the `deferFn` have been unified. They now share the following signature: ```ts export type AsyncFn<T, C> = ( context: C | undefined, props: AsyncProps<T, C>, controller: AbortController ) => Promise<T> ``` Before the `deferFn` and `promiseFn` had this signature: ```ts export type PromiseFn<T> = (props: AsyncProps<T>, controller: AbortController) => Promise<T> export type DeferFn<T> = ( args: any[], props: AsyncProps<T>, controller: AbortController ) => Promise<T> ``` The big change is the introduction of the `context` parameter. The idea behind this parameter is that it will contain the parameters which are not known to `AsyncOptions` for use in the `promiseFn` and `asyncFn`. Another goal of this commit is to make TypeScript more understanding of the `context` which `AsyncProps` implicitly carries around. Before this commit the `AsyncProps` accepted extra prop via `[prop: string]: any`. This breaks TypeScript's understanding of the divisions somewhat. This also led to missing types for `onCancel` and `suspense`, which have been added in this commit. To solve this we no longer allow random extra properties that are unknown to `AsyncProps`. Instead only the new `context` of `AsyncProps` is passed. This means that the `[prop: string]: any` of `AsyncProps` is removed this makes TypeScript understand the props better. The other big change of this commit that `useAsync` no longer supports an overload. This means that the user can no longer do: ```ts const state = useAsync(loadPlayer, { context: { playerId: 1 } }) ``` But have to be more explicit: ```t const state = useAsync({ promiseFn: loadPlayer, context: { playerId: 1 } }) ``` These changes are of course a breaking change. Also now compiling TypeScript on `yarn test` this should prevent type errors from slipping in. Closes: async-library#246 WIP: Trying to fix build asdf

moroine · 2020-07-21T05:50:11Z

What's the status of this PR?

ghengeveld · 2020-07-21T21:42:29Z

What's the status of this PR?

In need of a close review. It's been a while since I looked at it.

LRNZ09 · 2020-08-28T14:08:03Z

Just my 2c here: I like the fact that in v10 you can use the short version of the useAsync hook by calling it with the promise function as the first parameter. It's probably a poor choice moving it to the promiseFn key, being it always mandatory.

LRNZ09 · 2020-11-12T10:54:08Z

Any update on this?

ZebulanStanphill

I have some experience with TypeScript, so I decided to take a look at this PR.

I found some type weirdness in packages/react-async/src/useAsync.tsx, as well as some potential simplifications and wording improvements that could be made in other files. Also, this PR needs a rebase.

ZebulanStanphill · 2021-04-26T20:31:44Z

docs/api/options.md

@@ -5,6 +5,7 @@ These can be passed in an object to `useAsync(options)`, or as props to `<Async
 - [`promise`](#promise) An already started Promise instance.
 - [`promiseFn`](#promisefn) Function that returns a Promise, automatically invoked.
 - [`deferFn`](#deferfn) Function that returns a Promise, manually invoked with `run`.
+- [`context`](#context) The first argument for the `promise` and `promiseFn` function.


Suggested change

- [`context`](#context) The first argument for the `promise` and `promiseFn` function.

- [`context`](#context) The first argument for the `promiseFn` or `deferFn` function.

ZebulanStanphill · 2021-04-26T20:32:09Z

docs/api/options.md


 A function that returns a promise. It is automatically invoked in `componentDidMount` and `componentDidUpdate`. The function receives all component props \(or options\) and an AbortController instance as arguments.

-> Be aware that updating `promiseFn` will trigger it to cancel any pending promise and load the new promise. Passing an inline (arrow) function will cause it to change and reload on every render of the parent component. You can avoid this by defining the `promiseFn` value **outside** of the render method. If you need to pass variables to the `promiseFn`, pass them as additional props to `<Async>`, as `promiseFn` will be invoked with these props. Alternatively you can use `useCallback` or [memoize-one](https://github.com/alexreardon/memoize-one) to avoid unnecessary updates.
+> Be aware that updating `promiseFn` will trigger it to cancel any pending promise and load the new promise. Passing an inline (arrow) function will cause it to change and reload on every render of the parent component. You can avoid this by defining the `promiseFn` value **outside** of the render method. If you need to pass variables to the `promiseFn`, pass them via the `context` props of `<Async>`, as `promiseFn` will be invoked with these props. Alternatively you can use `useCallback` or [memoize-one](https://github.com/alexreardon/memoize-one) to avoid unnecessary updates.


Suggested change

> Be aware that updating `promiseFn` will trigger it to cancel any pending promise and load the new promise. Passing an inline (arrow) function will cause it to change and reload on every render of the parent component. You can avoid this by defining the `promiseFn` value **outside** of the render method. If you need to pass variables to the `promiseFn`, pass them via the `context` props of `<Async>`, as `promiseFn` will be invoked with these props. Alternatively you can use `useCallback` or [memoize-one](https://github.com/alexreardon/memoize-one) to avoid unnecessary updates.

> Be aware that updating `promiseFn` will trigger it to cancel any pending promise and load the new promise. Passing an inline (arrow) function will cause it to change and reload on every render of the parent component. You can avoid this by defining the `promiseFn` value **outside** of the render method. If you need to pass variables to the `promiseFn`, pass them via the `context` prop of `<Async>`, as `promiseFn` will be invoked with these props. Alternatively you can use `useCallback` or [memoize-one](https://github.com/alexreardon/memoize-one) to avoid unnecessary updates.

ZebulanStanphill · 2021-04-26T20:35:06Z

docs/getting-started/upgrading.md

+The difference is the idea of having a `context`, the context will contain all parameters
+to `AsyncProps` which are not native to the `AsyncProps`. Before you could pass any parameter
+to `AsyncProps` and it would pass them to the `deferFn` and `promiseFn`, now you need to use
+the `context` instead.


Suggested change

The difference is the idea of having a `context`, the context will contain all parameters

to `AsyncProps` which are not native to the `AsyncProps`. Before you could pass any parameter

to `AsyncProps` and it would pass them to the `deferFn` and `promiseFn`, now you need to use

the `context` instead.

The difference is the introduction of a `context` parameter. The context will contain all parameters

to `AsyncProps` which are not native to `AsyncProps`. Before you could pass any parameter

to `AsyncProps` and it would pass them to the `deferFn` and `promiseFn`; now you need to put them in the `context` instead.

ZebulanStanphill · 2021-04-26T20:40:37Z

docs/getting-started/usage.md

 ```jsx
 const MyComponent = () => {
-  const { data, error, isPending } = useAsync(loadPlayer, options)
+  const { data, error, isPending } = useAsync({ promiseFn: loadPlayer, context: { playerId: 1 } })
  // ...
 }
 ```


This example no longer makes any sense if you're going to just remove the shorthand version, so this entire section should be removed.

ZebulanStanphill · 2021-04-26T20:43:11Z

docs/guide/async-actions.md

@@ -36,11 +36,11 @@ const NewsletterForm = () => {
 }
 ```

-As you can see, the `deferFn` is invoked with 3 arguments: `args`, `props` and the AbortController. `args` is an array
+As you can see, the `deferFn` is invoked with 3 arguments: `context`, `props` and the AbortController. `context` is an object


Oxford comma.

Suggested change

As you can see, the `deferFn` is invoked with 3 arguments: `context`, `props` and the AbortController. `context` is an object

As you can see, the `deferFn` is invoked with 3 arguments: `context`, `props`, and the AbortController. `context` is an object

ZebulanStanphill · 2021-04-26T20:54:54Z

packages/react-async/src/useAsync.spec.js

        watch: count,
+        context: {
+          a: 1,


What's the purpose the a property in this test?

ZebulanStanphill · 2021-04-26T21:12:47Z

packages/react-async/src/useAsync.tsx

  const counter = useRef(0)
  const isMounted = useRef(true)
-  const lastArgs = useRef<any[] | undefined>(undefined)
-  const lastOptions = useRef<AsyncOptions<T>>(options)
+  const lastArgs = useRef<C | undefined>(undefined)


Shouldn't this just be:

Suggested change

const lastArgs = useRef<C | undefined>(undefined)

const lastArgs = useRef<C>(undefined)

I haven't tested, but I think this may remove the need to explicitly cast to C in a few places.

ZebulanStanphill · 2021-04-26T21:49:38Z

packages/react-async/src/useAsync.tsx

@@ -282,10 +273,10 @@ function isEvent(e: FetchRunArgs[0]): e is Event | React.SyntheticEvent {
 * @param {FetchOptions} options
 * @returns {AsyncState<T, FetchRun<T>>}
 */
-function useAsyncFetch<T>(
+function useAsyncFetch<T, C>(


The typing for this function is kinda wack.

First of all, the init parameter is using a RequestInit type that doesn't seem to actually exist. I think the type parameter C here should replace every instance of RequestInit... or rather, perhaps C should just be renamed to RequestInit here?

Second, the 3rd argument's type probably shouldn't be FetchOptions, but rather a subset of FetchOptions (that may not need the 2nd type parameter C at all), because passing in an object containing promiseFn or deferFn props doesn't make any sense in this context, yet it's technically allowed by the types here.

I found RequestInit defined in node_modules/@types/react-native/globals.d.ts. Appears to be a global type which should explain why it isn't declared in this file.
And I think C works just fine here as it's the type for the Context rather than the inits. So, it probably shouldn't replace any instance of RequestInit.

ZebulanStanphill · 2021-04-26T21:52:24Z

packages/react-async/src/useAsync.tsx

  const promiseFn = useCallback(
-    (_: AsyncOptions<T>, { signal }: AbortController) => {
+    (context: C, _: AsyncOptions<T, C>, { signal }: AbortController) => {


Since the 2nd parameter's name starts with an underscore to indicate it isn't used, the 1st one should do the same for consistency/clarity.

Suggested change

(context: C, _: AsyncOptions<T, C>, { signal }: AbortController) => {

(_context: C, _options: AsyncOptions<T, C>, { signal }: AbortController) => {

Also, my other comment about C and RequestInit being the same thing applies here as well.

Dysron · 2022-01-06T23:26:58Z

Any activity on this still? Looks like a great update to the package!

MrHus requested a review from ghengeveld January 21, 2020 10:30

ghengeveld self-assigned this Jan 21, 2020

ghengeveld requested a review from phryneas January 21, 2020 10:31

ghengeveld assigned phryneas Jan 21, 2020

ghengeveld reviewed Jan 21, 2020

View reviewed changes

freeman reviewed Jan 23, 2020

View reviewed changes

MrHus force-pushed the improvement/add-context-to-async-options branch from ef374bf to 57d2143 Compare January 27, 2020 09:14

MrHus force-pushed the improvement/add-context-to-async-options branch 3 times, most recently from c3dd34f to 131646e Compare January 27, 2020 13:24

MrHus force-pushed the improvement/add-context-to-async-options branch from 131646e to 2a45ae2 Compare January 27, 2020 13:29

ghengeveld mentioned this pull request Feb 26, 2020

Redefine useAsync to separate config async config from parameters to the promiseFn #246

Open

ghengeveld mentioned this pull request Jul 10, 2020

Typing issue with useAsync and TypeScript #256

Open

ZebulanStanphill suggested changes Apr 26, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement/add context to async options #247

Improvement/add context to async options #247

MrHus commented Jan 21, 2020

codesandbox-ci bot commented Jan 21, 2020 •

edited

Loading

ghengeveld Jan 21, 2020

MrHus Jan 21, 2020

ghengeveld Jan 21, 2020

ghengeveld commented Jan 21, 2020

freeman Jan 23, 2020

ghengeveld Jan 23, 2020

MrHus Jan 24, 2020

phryneas Jan 24, 2020

ghengeveld Jan 27, 2020

ghengeveld Jan 27, 2020

ghengeveld Jan 27, 2020 •

edited

Loading

MrHus Jan 27, 2020

codecov bot commented Jan 27, 2020 •

edited

Loading

moroine commented Jul 21, 2020

ghengeveld commented Jul 21, 2020

LRNZ09 commented Aug 28, 2020

LRNZ09 commented Nov 12, 2020

ZebulanStanphill left a comment

ZebulanStanphill Apr 26, 2021

ZebulanStanphill Apr 26, 2021

ZebulanStanphill Apr 26, 2021

ZebulanStanphill Apr 26, 2021

ZebulanStanphill Apr 26, 2021

ZebulanStanphill Apr 26, 2021

ZebulanStanphill Apr 26, 2021

ZebulanStanphill Apr 26, 2021

Dysron Jan 6, 2022

ZebulanStanphill Apr 26, 2021

Dysron commented Jan 6, 2022 •

edited

Loading

	- [`context`](#context) The first argument for the `promise` and `promiseFn` function.
	- [`context`](#context) The first argument for the `promiseFn` or `deferFn` function.

	As you can see, the `deferFn` is invoked with 3 arguments: `context`, `props` and the AbortController. `context` is an object
	As you can see, the `deferFn` is invoked with 3 arguments: `context`, `props`, and the AbortController. `context` is an object

	const lastArgs = useRef<C \| undefined>(undefined)
	const lastArgs = useRef<C>(undefined)

	(context: C, _: AsyncOptions<T, C>, { signal }: AbortController) => {
	(_context: C, _options: AsyncOptions<T, C>, { signal }: AbortController) => {

Improvement/add context to async options #247

Are you sure you want to change the base?

Improvement/add context to async options #247

Conversation

MrHus commented Jan 21, 2020

Description

Breaking changes

Checklist

codesandbox-ci bot commented Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghengeveld commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghengeveld Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2020 • edited Loading

Codecov Report

moroine commented Jul 21, 2020

ghengeveld commented Jul 21, 2020

LRNZ09 commented Aug 28, 2020

LRNZ09 commented Nov 12, 2020

ZebulanStanphill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dysron commented Jan 6, 2022 • edited Loading

codesandbox-ci bot commented Jan 21, 2020 •

edited

Loading

ghengeveld Jan 27, 2020 •

edited

Loading

codecov bot commented Jan 27, 2020 •

edited

Loading

Dysron commented Jan 6, 2022 •

edited

Loading