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

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

Open
MrHus opened this issue Jan 17, 2020 · 5 comments

Comments

@MrHus
Copy link

MrHus commented Jan 17, 2020

This is a proposal for a change in the way useAsync works. I propose to add a third parameter, which is an object which is passed as the first argument to the promiseFn.

Currently this information is passed along in the AsyncOptions as [prop: string]: any:

export interface AsyncOptions<T> {
  promise?: Promise<T>
  promiseFn?: PromiseFn<T>
  // abbreviated
  debugLabel?: string
  [prop: string]: any
}

This makes the arguments to the promiseFn unsafe. Which is not nice for the developer experience. Plus it makes the AsyncOptions have a strange dual role.

My Proposal would create a dedicated parameter for the parameters to the promiseFn function like so :

type LoadDataConfig = {
  id: string
}

export function loadData(config: LoadDataConfig, props: AsyncProps<Link>, controller: AbortController) {
  return Link.one(config.id);
}

export default function AwesomeComponent() {
  const { id } = useParams();
  const state = useAsync(loadLink, { id }, { watch: id });
}

This would also mean redefining PromiseFn like so:

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

Another less breaking options would be to add the config to AsyncOptions:

export interface AsyncOptions<T, C> {
  promise?: Promise<T>
  promiseFn?: PromiseFn<T>
  // abbreviated
  debugLabel?: string
config: C
  [prop: string]: any
}

This would at least make the config type safe, but makes the api a little nicer.

@ghengeveld
Copy link
Member

I'd really love to make the additional props type safe, so I like your proposal. Let's try to come up with a solution that solves multiple problems we have right now.

I prefer to avoid introducing any API changes, such as adding an extra argument at the start of the argument list for PromiseFn. However, I'd also like to get rid of the difference in arguments between PromiseFn and DeferFn (which receives one more argument). Maybe this is an opportunity to fix that too.

That would mean DeferFn and PromiseFn both receive 3 arguments, the first being something of type C (let's call it context). For PromiseFn it would be the thing you passed to useAsync and for DeferFn it would be the thing you passed to run.

function(context: C, props: Object, controller: AbortController): Promise<T>

We can't easily add an extra argument to useAsync because it's already overloaded. I mean we could but I prefer not to make it even more complicated. Instead let's just add context on AsyncOptions.

Would you be willing to setup a PR for this?

@MrHus
Copy link
Author

MrHus commented Jan 20, 2020

I'll try to make a PR.

MrHus added a commit to MrHus/react-async that referenced this issue Jan 21, 2020
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 all extra properties that are unknown to `AsyncProps` are
put on the `AsyncProps`'s `context` property. This means that when
using TypeScript you can now use `context` to safely extract the context.
This does however mean that because `[prop: string]: any` is removed
TypeScript users have a breaking change.

Closes: async-library#246
MrHus added a commit to MrHus/react-async that referenced this issue Jan 21, 2020
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 all extra properties that are unknown to `AsyncProps` are
put on the `AsyncProps`'s `context` property. This means that when
using TypeScript you can now use `context` to safely extract the context.
This does however mean that because `[prop: string]: any` is removed
TypeScript users have a breaking change.

Closes: async-library#246
MrHus added a commit to MrHus/react-async that referenced this issue Jan 27, 2020
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.

This is of course a breaking change.

Also now compiling TypeScript on `yarn test` this should prevent type
errors from slipping in.

Closes: async-library#246
MrHus added a commit to MrHus/react-async that referenced this issue Jan 27, 2020
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.

This is of course a breaking change.

Also now compiling TypeScript on `yarn test` this should prevent type
errors from slipping in.

Closes: async-library#246
MrHus added a commit to MrHus/react-async that referenced this issue Jan 27, 2020
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
MrHus added a commit to MrHus/react-async that referenced this issue Jan 27, 2020
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
@samit4me
Copy link

This looks like it might also resolve #256

Is there plans to make this happen or is it still just an idea at the moment?

@ghengeveld
Copy link
Member

I'm still down for this change. Just swamped at the moment.

@ghengeveld
Copy link
Member

There's this PR by the way: #247

Would be great if someone could review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants