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

feat(signal): remove passed signal from defautlMapOptionsToKey #83

Merged
merged 5 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,31 @@
};
```

### Abort Signal
Copy link
Contributor

@JAdshead JAdshead Aug 21, 2023

Choose a reason for hiding this comment

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

Im not sure that this should be documented in this mannor, else the readme should document all of the fetch apis.
Maybe this should be framed as an example of aborting an inflight request on unmount to demonstraight that useFetchye is just a wrapper around fetch.


When you neeed to abort the execution of a `useFetchye` call, you may
pass a signal as an option in such way.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pass a signal as an option in such way.
pass a signal as an option, as demonstrated below.


```jsx
import React, { useEffect } from 'react';
import { useFetchye } from 'fetchye';

const AbortComponent = () => {
const controller = new AbortController();
useFetchye('http://example.com/api/books', { signal: controller.signal });

useEffect(() => () => controller.abort(), []);

Check warning on line 353 in README.md

View workflow job for this annotation

GitHub Actions / Node 12.x

React Hook useEffect has a missing dependency: 'controller'. Either include it or remove the dependency array

Check warning on line 353 in README.md

View workflow job for this annotation

GitHub Actions / Node 14.x

React Hook useEffect has a missing dependency: 'controller'. Either include it or remove the dependency array

Check warning on line 353 in README.md

View workflow job for this annotation

GitHub Actions / Node 16.x

React Hook useEffect has a missing dependency: 'controller'. Either include it or remove the dependency array

Check warning on line 353 in README.md

View workflow job for this annotation

GitHub Actions / Node 18.x

React Hook useEffect has a missing dependency: 'controller'. Either include it or remove the dependency array

return (
<div>
<h1>abortable component</h1>
</div>
);
};
```
Instead of setting up a `useEffect` within the component it's possible to pass a hook to signal using packages such as
[use-unmount-signal](https://www.npmjs.com/package/use-unmount-signal/v/1.0.0).

### Sequential API Execution

Passing the 'isLoading' value from one useFetchye call to the 'defer' field of the next will prevent the second call from being made until the first has loaded.
Expand Down Expand Up @@ -748,7 +773,7 @@
**Shape**

```
const { isLoading, data, error, run } = useFetchye(key, { defer: Boolean, mapOptionsToKey: options => options, ...fetchOptions }, fetcher);
const { isLoading, data, error, run } = useFetchye(key, { signal: {}, defer: Boolean, mapOptionsToKey: options => options, ...fetchOptions }, fetcher);
Copy link
Contributor

@JAdshead JAdshead Aug 21, 2023

Choose a reason for hiding this comment

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

signal is a fetch api that would be included as part of fetchOptions and is not part of the useFetchye api so should be removed from here.

Suggested change
const { isLoading, data, error, run } = useFetchye(key, { signal: {}, defer: Boolean, mapOptionsToKey: options => options, ...fetchOptions }, fetcher);
const { isLoading, data, error, run } = useFetchye(key, { defer: Boolean, mapOptionsToKey: options => options, ...fetchOptions }, fetcher);

```

**Arguments**
Expand All @@ -765,6 +790,7 @@
|--------------------|-------------------------------------------------------|----------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `mapOptionsToKey` | `(options: Options) => transformedOptions` | `false` | A function that maps options to the key that will become part of the cache key |
| `mapKeyToCacheKey` | `(key: String, options: Options) => cacheKey: String` | `false` | A function that maps the key for use as the cacheKey allowing direct control of the cacheKey |
| `signal` | `Object` | `false` | Prevents execution of `useFetchye` when desired cancelling DOM request (See [Abort Controller](https://developer.mozilla.org/en-US/docs/Web/API/AbortController)). |
Copy link
Contributor

@JAdshead JAdshead Aug 21, 2023

Choose a reason for hiding this comment

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

This is not a useFetchye api so should be removed.

Suggested change
| `signal` | `Object` | `false` | Prevents execution of `useFetchye` when desired cancelling DOM request (See [Abort Controller](https://developer.mozilla.org/en-US/docs/Web/API/AbortController)). |

| `defer` | `Boolean` | `false` | Prevents execution of `useFetchye` on each render in favor of using the returned `run` function. *Defaults to `false`* |
| `initialData` | `Object` | `false` | Seeds the initial data on first render of `useFetchye` to accomodate server side rendering *Defaults to `undefined`* |
| `...restOptions` | `ES6FetchOptions` | `true` | Contains any ES6 Compatible `fetch` option. (See [Fetch Options](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#Supplying_request_options)) |
Expand Down
6 changes: 4 additions & 2 deletions packages/fetchye/__tests__/defaultMapOptionsToKey.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
import { defaultMapOptionsToKey } from '../src/defaultMapOptionsToKey';

describe('defaultMapOptionsToKey', () => {
it('should return an object without passed defer or mapOptionsToKey', () => {
expect(defaultMapOptionsToKey({ defer: true, mapOptionsToKey: () => {}, method: 'POST' }))
it('should return an object without passed signal, defer, or mapOptionsToKey', () => {
expect(defaultMapOptionsToKey({
signal: {}, defer: true, mapOptionsToKey: () => { }, method: 'POST',
}))
.toMatchInlineSnapshot(`
Object {
"method": "POST",
Expand Down
1 change: 1 addition & 0 deletions packages/fetchye/src/defaultMapOptionsToKey.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

export const defaultMapOptionsToKey = (options) => {
const {
signal,
Copy link
Member

Choose a reason for hiding this comment

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

Can you make a unit tests that calls this function with this name and shows that it is removed.

One that would fail if this change was reverted

defer,
mapOptionsToKey,
initialData,
Expand Down
Loading