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

Browser Support, Memory Leak Fixes #210

Merged
merged 4 commits into from
Mar 23, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
35 changes: 20 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
<a href="http://use-http.com">
<img src="https://github.com/alex-cory/use-http/raw/master/public/dog.png" />
</a>
[![use-http logo][3]][5]

<br/>

Expand Down Expand Up @@ -875,10 +873,19 @@ Does your company use use-http? Consider sponsoring the project to fund new feat
</a>
</p>

Browser Support
---------------

If you need support for IE, you will need to add additional polyfills. The React docs suggest [these polyfills][4], but from [this issue][2] we have found it to work fine with the [`react-app-polyfill`]. If you have any updates to this browser list, please submit a PR!

| [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/edge/edge_48x48.png" alt="IE / Edge" width="24px" height="24px" />]()<br/>Edge | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/firefox/firefox_48x48.png" alt="Firefox" width="24px" height="24px" />]()<br/>Firefox | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/chrome/chrome_48x48.png" alt="Chrome" width="24px" height="24px" />]()<br/>Chrome | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/safari/safari_48x48.png" alt="Safari" width="24px" height="24px" />]()<br/>Safari | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/opera/opera_48x48.png" alt="Opera" width="24px" height="24px" />]()<br/>Opera |
| --------- | --------- | --------- | --------- | --------- | --------- | --------- |
| 12+ | last 2 versions| last 2 versions| last 2 versions| last 2 versions| last 2 versions| last 2 versions

Feature Requests/Ideas
----------------------

If you have feature requests, let's talk about them in [this issue](https://github.com/alex-cory/use-http/issues/13)!
If you have feature requests, [submit an issue][1] to let us know what you would like to see!

Todos
------
Expand All @@ -893,7 +900,6 @@ Todos
- snapshot test resources: [swr](https://github.com/zeit/swr/blob/master/test/use-swr.test.tsx#L1083), [react-apollo-hooks](https://github.com/trojanowski/react-apollo-hooks/blob/master/src/__tests__/useQuery-test.tsx#L218)
- basic test resources: [fetch-suspense](https://github.com/CharlesStover/fetch-suspense/blob/master/tests/create-use-fetch.test.ts), [@testing-library/react-hooks suspense PR](https://github.com/testing-library/react-hooks-testing-library/pull/35/files)
- [ ] maybe add translations [like this one](https://github.com/jamiebuilds/unstated-next)
- [ ] add browser support to docs [1](https://github.com/godban/browsers-support-badges) [2](https://gist.github.com/danbovey/b468c2f810ae8efe09cb5a6fac3eaee5) (currently does not support ie 11)
- [ ] maybe add contributors [all-contributors](https://github.com/all-contributors/all-contributors)
- [ ] add sponsors [similar to this](https://github.com/carbon-app/carbon)
- [ ] Error handling
Expand All @@ -908,6 +914,7 @@ Todos
- [ ] aborts fetch on unmount
- [ ] take a look at how [react-apollo-hooks](https://github.com/trojanowski/react-apollo-hooks) work. Maybe ad `useSubscription` and `const request = useFetch(); request.subscribe()` or something along those lines
- [ ] make this a github package
- [see ava packages](https://github.com/orgs/ava/packages)
- [ ] get it all working on a SSR codesandbox, this way we can have api to call locally
- [ ] make GraphQL examples in codesandbox
- [ ] Documentation:
Expand All @@ -930,16 +937,6 @@ Todos
.get()
```

- [ ] maybe add snake_case -> camelCase option to `<Provider />`. This would
convert all the keys in the response to camelCase.
Not exactly sure how this syntax should look because what
if you want to have this only go 1 layer deep into the response
object. Or if this is just out of scope for this library.

```jsx
<Provider responseKeys={{ case: 'camel' }}><App /></Provider>
```

- [ ] potential option ideas

```jsx
Expand Down Expand Up @@ -1042,3 +1039,11 @@ const App = () => {
}
```
</details>


[1]: https://github.com/alex-cory/use-http/issues/new?title=[Feature%20Request]%20YOUR_FEATURE_NAME
[2]: https://github.com/alex-cory/use-http/issues/93#issuecomment-600896722
[3]: https://github.com/alex-cory/use-http/raw/master/public/dog.png
[4]: https://reactjs.org/docs/javascript-environment-requirements.html
[5]: http://use-http.com
[`react-app-polyfill`]: https://www.npmjs.com/package/react-app-polyfill
29 changes: 25 additions & 4 deletions docs/README.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<img src="https://github.com/alex-cory/use-http/raw/master/public/dog.png" />
![use-http logo][3]

<p align="center">
<h1 align="center">useFetch</h1>
Expand Down Expand Up @@ -821,6 +821,7 @@ useFetch(options)

Who's using use-http?
=====================

<div style="display: flex; align-items: center; justify-content: center;">
<a href="https://ava.inc">
<img height="140px" src="https://github.com/alex-cory/use-http/raw/master/public/ava-logo.png" />
Expand All @@ -836,11 +837,23 @@ Who's using use-http?
</a>
</div>

Browser Support
===============

If you need support for IE, you will need to add additional polyfills. The React docs suggest [these polyfills][4], but from [this issue][2] we have found it to work fine with the [`react-app-polyfill`]. If you have any updates to this browser list, please submit a PR!

| [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/edge/edge_48x48.png" alt="IE / Edge" width="24px" height="24px" />]()<br/>Edge | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/firefox/firefox_48x48.png" alt="Firefox" width="24px" height="24px" />]()<br/>Firefox | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/chrome/chrome_48x48.png" alt="Chrome" width="24px" height="24px" />]()<br/>Chrome | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/safari/safari_48x48.png" alt="Safari" width="24px" height="24px" />]()<br/>Safari | [<img src="https://raw.githubusercontent.com/alrra/browser-logos/master/src/opera/opera_48x48.png" alt="Opera" width="24px" height="24px" />]()<br/>Opera |
| --------- | --------- | --------- | --------- | --------- | --------- | --------- |
| 12+ | last 2 versions| last 2 versions| last 2 versions| last 2 versions| last 2 versions| last 2 versions

Feature Requests/Ideas
======================
If you have feature requests, let's talk about them in [this issue](https://github.com/alex-cory/use-http/issues/13)!

<!-- Mutations with Suspense <sup>(Not Implemented Yet)</sup>
If you have feature requests, [submit an issue][1] to let us know what you would like to see!

<!--

Mutations with Suspense <sup>(Not Implemented Yet)</sup>
==================
```js
const App = () => {
Expand Down Expand Up @@ -868,4 +881,12 @@ const App = () => {
</>
)
}
``` -->
```

-->

[1]: https://github.com/alex-cory/use-http/issues/new?title=[Feature%20Request]%20YOUR_FEATURE_NAME
[2]: https://github.com/alex-cory/use-http/issues/93#issuecomment-600896722
[3]: https://github.com/alex-cory/use-http/raw/master/public/dog.png
[4]: https://reactjs.org/docs/javascript-environment-requirements.html
[`react-app-polyfill`]: https://www.npmjs.com/package/react-app-polyfill
7 changes: 4 additions & 3 deletions src/__tests__/useFetch.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ describe('useFetch - BROWSER - suspense', (): void => {
afterEach((): void => {
fetch.resetMocks()
cleanup()

test.cleanup()
})

Expand Down Expand Up @@ -939,7 +940,6 @@ describe('useFetch - BROWSER - persistence', (): void => {
expect(fetch).toHaveBeenCalledTimes(1)
})


it('should have `cache` in the return of useFetch', async (): Promise<void> => {
const { result } = renderHook(
() => useFetch({ url: 'https://persist.com', persist: true }, [])
Expand All @@ -953,14 +953,15 @@ describe('useFetch - BROWSER - persistence', (): void => {
})

it('should error if passing wrong cachePolicy with persist: true', async (): Promise<void> => {

try {
const { result } = renderHook(
() => useFetch({ url: 'https://persist.com', persist: true, cachePolicy: NO_CACHE }, [])
)
expect(result.current.error).toBe(undefined)
} catch (err) {
expect(err.name).toBe('Invariant Violation')
expect(err.message).toBe(`You cannot use option 'persist' with cachePolicy: no-cache 🙅‍♂️`)
expect(err.message).toBe('You cannot use option \'persist\' with cachePolicy: no-cache 🙅‍♂️')
}

try {
Expand All @@ -970,7 +971,7 @@ describe('useFetch - BROWSER - persistence', (): void => {
expect(result.current.error).toBe(undefined)
} catch (err) {
expect(err.name).toBe('Invariant Violation')
expect(err.message).toBe(`You cannot use option 'persist' with cachePolicy: network-only 🙅‍♂️`)
expect(err.message).toBe('You cannot use option \'persist\' with cachePolicy: network-only 🙅‍♂️')
}
})
})
2 changes: 1 addition & 1 deletion src/doFetchArgs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export default async function doFetchArgs<TData = any>(

// TODO: see if `Object.entries` is supported for IE
// TODO: if the body is a file, and this is a large file, it might exceed the size
// limit of the key size in the Map
// limit of the key size. Potential solution: base64 the body
// used to tell if a request has already been made
const responseID = Object.entries({ url, method, body: options.body || '' })
.map(([key, value]) => `${key}:${value}`).join('||')
Expand Down
43 changes: 21 additions & 22 deletions src/storage/localStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,26 @@ const getCache = () => {
}
}
const getLocalStorage = ({ cacheLife }: { cacheLife: number }): Cache => {
// there isn't state here now, but will be eventually

const remove = async (name: string) => {
const isExpired = (responseID: string) => {
const cache = getCache()
delete cache[name]
localStorage.setItem(cacheName, JSON.stringify(cache))
const { expiration, response } = (cache[responseID] || {})
const expired = expiration > 0 && expiration < Date.now()
if (expired) remove(responseID)
return expired || !response
}

const has = async (responseID: string): Promise<boolean> => {
const remove = async (...responseIDs: string[]) => {
const cache = getCache()
return !!(cache[responseID] && cache[responseID].response)
responseIDs.forEach(id => delete cache[id])
localStorage.setItem(cacheName, JSON.stringify(cache))
}

const get = async (responseID: string): Promise<Response | undefined> => {
const cache = getCache()
if (!cache[responseID]) {
return
}

const { expiration, response: { body, headers, status, statusText } } = cache[responseID] as any
if (expiration < Date.now()) {
delete cache[responseID]
localStorage.setItem(cacheName, JSON.stringify(cache))
return
}
const has = async (responseID: string) => !isExpired(responseID)

const get = async (responseID: string) => {
if (isExpired(responseID)) return
const cache = getCache()
const { body, headers, status, statusText } = cache[responseID].response
return new Response(body, {
status,
statusText,
Expand All @@ -47,9 +41,8 @@ const getLocalStorage = ({ cacheLife }: { cacheLife: number }): Cache => {

const set = async (responseID: string, response: Response): Promise<void> => {
const cache = getCache()
const responseObject = await serializeResponse(response)
cache[responseID] = {
response: responseObject,
response: await serializeResponse(response),
expiration: Date.now() + cacheLife
}
localStorage.setItem(cacheName, JSON.stringify(cache))
Expand All @@ -59,7 +52,13 @@ const getLocalStorage = ({ cacheLife }: { cacheLife: number }): Cache => {
localStorage.setItem(cacheName, JSON.stringify({}))
}

return { get, set, has, delete: remove, clear }
return Object.defineProperties(getCache(), {
get: { value: get, writable: false },
set: { value: set, writable: false },
has: { value: has, writable: false },
delete: { value: remove, writable: false },
clear: { value: clear, writable: false }
})
}

export default getLocalStorage
64 changes: 37 additions & 27 deletions src/storage/memoryStorage.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,44 @@
import { Cache } from '../types'

const inMemoryStorage = new Map<string, Response | number>()
const getMemoryStorage = ({ cacheLife }: { cacheLife: number }): Cache => ({
async get(name: string) {
const item = inMemoryStorage.get(name) as Response | undefined
if (!item) return

const expiration = inMemoryStorage.get(`${name}:ts`)
if (expiration && expiration > 0 && expiration < Date.now()) {
inMemoryStorage.delete(name)
inMemoryStorage.delete(`${name}:ts`)
return
var inMemoryStorage: any = {}
var getMemoryStorage = ({ cacheLife }: { cacheLife: number }): Cache => {
const isExpired = (responseID: string) => {
const expiration = inMemoryStorage[`${responseID}:ts`]
const expired = expiration > 0 && expiration < Date.now()
if (expired) remove(responseID)
return expired
}

const get = async (responseID: string) => {
if (isExpired(responseID)) return
return inMemoryStorage[responseID] as Response
}

const set = async (responseID: string, res: Response) => {
inMemoryStorage[responseID] = res
inMemoryStorage[`${responseID}:ts`] = cacheLife > 0 ? Date.now() + cacheLife : 0
}

const has = async (responseID: string) => !isExpired(responseID)

const remove = async (...responseIDs: string[]) => {
for (const responseID of responseIDs) {
delete inMemoryStorage[responseID]
delete inMemoryStorage[`${responseID}:ts`]
}
}

return item
},
async set(name: string, data: Response) {
inMemoryStorage.set(name, data)
inMemoryStorage.set(`${name}:ts`, cacheLife > 0 ? Date.now() + cacheLife : 0)
},
async has(name: string) {
return inMemoryStorage.has(name)
},
async delete(name: string) {
inMemoryStorage.delete(name)
inMemoryStorage.delete(`${name}:ts`)
},
async clear() {
return inMemoryStorage.clear()
const clear = async () => {
inMemoryStorage = {}
}
})

return Object.defineProperties(inMemoryStorage, {
get: { value: get, writable: false, configurable: true },
set: { value: set, writable: false, configurable: true },
has: { value: has, writable: false, configurable: true },
delete: { value: remove, writable: false, configurable: true },
clear: { value: clear, writable: false, configurable: true }
})
}

export default getMemoryStorage
1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ export type Interceptors = {
response?: (response: Res<any>) => Res<any>
}

// this also holds the response keys. It mimics js Map
export type Cache = {
get: (name: string) => Promise<Response | undefined>
set: (name: string, data: Response) => Promise<void>
Expand Down
11 changes: 7 additions & 4 deletions src/useFetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ function useFetch<TData = any>(...args: UseFetchArgs): UseFetch<TData> {
const hasMore = useRef(true)
const suspenseStatus = useRef('pending')
const suspender = useRef<Promise<any>>()
const mounted = useRef(false)

const [loading, setLoading] = useState<boolean>(defaults.loading)
const forceUpdate = useReducer(() => ({}), [])[1]
Expand Down Expand Up @@ -78,19 +79,19 @@ function useFetch<TData = any>(...args: UseFetchArgs): UseFetch<TData> {
interceptors.request
)

if (!suspense) setLoading(true)
if (!suspense && mounted.current) setLoading(true)
error.current = undefined

if (response.isCached && cachePolicy === CACHE_FIRST) {
try {
res.current = response.cached as Res<TData>
res.current.data = await tryGetData(response.cached, defaults.data)
data.current = res.current.data as TData
if (!suspense) setLoading(false)
if (!suspense && mounted.current) setLoading(false)
return data.current
} catch (err) {
error.current = err
setLoading(false)
if (mounted.current) setLoading(false)
}
}

Expand Down Expand Up @@ -134,7 +135,7 @@ function useFetch<TData = any>(...args: UseFetchArgs): UseFetch<TData> {
controller.current = undefined
}

if (!suspense) setLoading(false)
if (!suspense && mounted.current) setLoading(false)

return data.current
} // end of doFetch()
Expand Down Expand Up @@ -182,12 +183,14 @@ function useFetch<TData = any>(...args: UseFetchArgs): UseFetch<TData> {

// onMount/onUpdate
useEffect((): any => {
mounted.current = true
if (Array.isArray(dependencies)) {
const methodName = requestInit.method || HTTPMethod.GET
const methodLower = methodName.toLowerCase() as keyof ReqMethods
const req = request[methodLower] as NoArgs
req()
}
return () => mounted.current = false
// TODO: need [request] in dependency array. Causing infinite loop though.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, dependencies)
Expand Down