-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Fix race condition in WithPermissions #5822
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
import * as React from 'react'; | ||
import { render, act, cleanup } from '@testing-library/react'; | ||
import expect from 'expect'; | ||
import usePermissionsOptimized from './usePermissionsOptimized'; | ||
import AuthContext from './AuthContext'; | ||
|
||
describe('usePermissionsOptimized', () => { | ||
afterEach(cleanup); | ||
|
||
const CallPermissionsOnMount = ({ number, authParams }: any) => { | ||
const { permissions } = usePermissionsOptimized(authParams); | ||
return ( | ||
<div> | ||
permissions {number}: {permissions} | ||
</div> | ||
); | ||
}; | ||
|
||
it('returns undefined on mount', () => { | ||
const getPermissions = jest.fn(() => Promise.resolve('admin')); | ||
const { queryByText } = render( | ||
<AuthContext.Provider value={{ getPermissions } as any}> | ||
<div> | ||
<CallPermissionsOnMount authParams={{ test: 1 }} /> | ||
</div> | ||
</AuthContext.Provider> | ||
); | ||
expect(queryByText('permissions :')).not.toBeNull(); | ||
expect(queryByText('permissions : admin')).toBeNull(); | ||
}); | ||
|
||
it('returns permissions from authProvider after resolve', async () => { | ||
const getPermissions = jest.fn(() => Promise.resolve('admin')); | ||
const { queryByText } = render( | ||
<AuthContext.Provider value={{ getPermissions } as any}> | ||
<div> | ||
<CallPermissionsOnMount authParams={{ test: 2 }} /> | ||
</div> | ||
</AuthContext.Provider> | ||
); | ||
await act(async () => await new Promise(r => setTimeout(r))); | ||
expect(queryByText('permissions :')).toBeNull(); | ||
expect(queryByText('permissions : admin')).not.toBeNull(); | ||
}); | ||
|
||
it('does not rerender once the permissions have already been fetched', async () => { | ||
let renders = 0; | ||
const ComponentToTest = () => { | ||
const { permissions } = usePermissionsOptimized({ test: 3 }); | ||
renders++; | ||
return <div>{permissions}</div>; | ||
}; | ||
const getPermissions = jest.fn(() => Promise.resolve('admin')); | ||
|
||
// first usage | ||
const { queryByText } = render( | ||
<AuthContext.Provider value={{ getPermissions } as any}> | ||
<ComponentToTest /> | ||
</AuthContext.Provider> | ||
); | ||
expect(renders).toBe(1); // renders on mount | ||
expect(getPermissions).toBeCalledTimes(1); | ||
expect(queryByText('admin')).toBeNull(); | ||
await act(async () => await new Promise(r => setTimeout(r))); | ||
expect(renders).toBe(2); // rerenders when the getPermissions returns | ||
expect(queryByText('admin')).not.toBeNull(); | ||
|
||
// second usage | ||
cleanup(); | ||
renders = 0; | ||
const { queryByText: queryByText2 } = render( | ||
<AuthContext.Provider value={{ getPermissions } as any}> | ||
<ComponentToTest /> | ||
</AuthContext.Provider> | ||
); | ||
expect(renders).toBe(1); // renders on mount | ||
expect(getPermissions).toBeCalledTimes(2); | ||
expect(queryByText2('admin')).not.toBeNull(); // answer from the cache | ||
await act(async () => await new Promise(r => setTimeout(r))); | ||
expect(renders).toBe(1); // does not rerender when the getPermissions returns the same permissions | ||
}); | ||
|
||
it('can be called by two independent components', async () => { | ||
const getPermissions = jest.fn(() => Promise.resolve('admin')); | ||
const { queryByText } = render( | ||
<AuthContext.Provider value={{ getPermissions } as any}> | ||
<div> | ||
<CallPermissionsOnMount | ||
number={1} | ||
authParams={{ test: 4 }} | ||
/> | ||
<CallPermissionsOnMount | ||
number={2} | ||
authParams={{ test: 4 }} | ||
/> | ||
</div> | ||
</AuthContext.Provider> | ||
); | ||
expect(queryByText('permissions 1:')).not.toBeNull(); | ||
expect(queryByText('permissions 2:')).not.toBeNull(); | ||
expect(queryByText('permissions 1: admin')).toBeNull(); | ||
expect(queryByText('permissions 2: admin')).toBeNull(); | ||
expect(getPermissions).toBeCalledTimes(2); | ||
await act(async () => await new Promise(r => setTimeout(r))); | ||
expect(queryByText('permissions 1:')).toBeNull(); | ||
expect(queryByText('permissions 2:')).toBeNull(); | ||
expect(queryByText('permissions 1: admin')).not.toBeNull(); | ||
expect(queryByText('permissions 2: admin')).not.toBeNull(); | ||
expect(getPermissions).toBeCalledTimes(2); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ const emptyParams = {}; | |
|
||
// keep a cache of already fetched permissions to initialize state for new | ||
// components and avoid a useless rerender if the permissions haven't changed | ||
const alreadyFetchedPermissions = { '{}': [] }; | ||
const alreadyFetchedPermissions = { '{}': undefined }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was another regression introduced in 3.11. Before, WithPermissions injected |
||
|
||
/** | ||
* Hook for getting user permissions without the loading state. | ||
|
@@ -53,15 +53,15 @@ const alreadyFetchedPermissions = { '{}': [] }; | |
* }; | ||
*/ | ||
const usePermissionsOptimized = (params = emptyParams) => { | ||
const key = JSON.stringify(params); | ||
const [state, setState] = useSafeSetState<State>({ | ||
permissions: alreadyFetchedPermissions[JSON.stringify(params)], | ||
permissions: alreadyFetchedPermissions[key], | ||
}); | ||
const getPermissions = useGetPermissions(); | ||
useEffect(() => { | ||
getPermissions(params) | ||
.then(permissions => { | ||
const key = JSON.stringify(params); | ||
if (!isEqual(permissions, alreadyFetchedPermissions[key])) { | ||
if (!isEqual(permissions, state.permissions)) { | ||
alreadyFetchedPermissions[key] = permissions; | ||
setState({ permissions }); | ||
} | ||
|
@@ -71,7 +71,7 @@ const usePermissionsOptimized = (params = emptyParams) => { | |
error, | ||
}); | ||
}); | ||
}, [getPermissions, params, setState]); | ||
}, [getPermissions, key]); // eslint-disable-line react-hooks/exhaustive-deps | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is done on purpose, and there is no way around it |
||
|
||
return state; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test used to fail before the patch