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.

Sign up for GitHub

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

[RFR] Ensure Query, Mutation and withDataProvider are backward compatible #3605

Merged
merged 11 commits into from
Aug 28, 2019
+613 −12
Merged

[RFR] Ensure Query, Mutation and withDataProvider are backward compatible #3605

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
da4f8b8
[WIP] Ensure Query, Mutation and withDataProvider are backward compat…
djhi Aug 26, 2019
82a7ca0
Fix unselectAll usage
djhi Aug 26, 2019
a0156bb
Add test for Query
djhi Aug 26, 2019
bde400c
Add tests for hooks based sideEffect on Query
djhi Aug 26, 2019
a1fc626
Added tests for Mutation
djhi Aug 26, 2019
2076680
Better check of side effects type
djhi Aug 28, 2019
b50824c
Add comments to useDataProviderWithDeclarativeSideEffects
djhi Aug 28, 2019
ca37b30
Review
djhi Aug 28, 2019
f1144e5
Refactor to avoid duplication
djhi Aug 28, 2019
558dbe3
Move Mutation tests
djhi Aug 28, 2019
22bc4aa
Use warning
djhi Aug 28, 2019
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
8 changes: 7 additions & 1 deletion packages/ra-core/src/dataProvider/Mutation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ const Mutation: FunctionComponent<Props> = ({
resource,
payload,
options,
}) => children(...useMutation({ type, resource, payload }, options));
}) =>
children(
...useMutation(
{ type, resource, payload },
{ ...options, withDeclarativeSideEffectsSupport: true }
)
);

export default Mutation;
244 changes: 244 additions & 0 deletions packages/ra-core/src/dataProvider/Query.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,16 @@ import {
waitForDomChange,
} from '@testing-library/react';
import expect from 'expect';
import { push } from 'connected-react-router';

import Query from './Query';
import CoreAdmin from '../CoreAdmin';
import Resource from '../Resource';
import renderWithRedux from '../util/renderWithRedux';
import TestContext from '../util/TestContext';
import DataProviderContext from './DataProviderContext';
import { showNotification, refreshView, setListSelectedIds } from '../actions';
import { useNotify } from '../sideEffect';

describe('Query', () => {
afterEach(cleanup);
Expand Down Expand Up @@ -251,4 +256,243 @@ describe('Query', () => {
});
expect(dispatchSpy.mock.calls.length).toEqual(3);
});

it('supports declarative onSuccess side effects', async () => {
expect.assertions(4);
let dispatchSpy;
const dataProvider = jest.fn();
dataProvider.mockImplementationOnce(() =>
Promise.resolve({ data: [{ id: 1, foo: 'bar' }], total: 42 })
);

let getByTestId;
act(() => {
const res = render(
<DataProviderContext.Provider value={dataProvider}>
<TestContext>
{({ store }) => {
dispatchSpy = jest.spyOn(store, 'dispatch');
return (
<Query
type="GET_LIST"
resource="foo"
options={{
onSuccess: {
notification: {
body: 'Youhou!',
level: 'info',
},
redirectTo: '/a_path',
refresh: true,
unselectAll: true,
},
}}
>
{({ loading, data, total }) => (
<div
data-testid="test"
className={
loading ? 'loading' : 'idle'
}
>
{loading ? 'no data' : total}
</div>
)}
</Query>
);
}}
</TestContext>
</DataProviderContext.Provider>
);
getByTestId = res.getByTestId;
});

const testElement = getByTestId('test');
await waitForDomChange({ container: testElement });

expect(dispatchSpy).toHaveBeenCalledWith(
showNotification('Youhou!', 'info', {
messageArgs: {},
undoable: false,
})
);
expect(dispatchSpy).toHaveBeenCalledWith(push('/a_path'));
expect(dispatchSpy).toHaveBeenCalledWith(refreshView());
expect(dispatchSpy).toHaveBeenCalledWith(setListSelectedIds('foo', []));
});

it('supports onSuccess function for side effects', async () => {
let dispatchSpy;
const dataProvider = jest.fn();
dataProvider.mockImplementationOnce(() =>
Promise.resolve({ data: [{ id: 1, foo: 'bar' }], total: 42 })
);

const Foo = () => {
const notify = useNotify();
return (
<Query
type="GET_LIST"
resource="foo"
options={{
onSuccess: () => {
notify('Youhou!', 'info');
},
}}
>
{({ loading, data, total }) => (
<div
data-testid="test"
className={loading ? 'loading' : 'idle'}
>
{loading ? 'no data' : total}
</div>
)}
</Query>
);
};
let getByTestId;
act(() => {
const res = render(
<DataProviderContext.Provider value={dataProvider}>
<TestContext>
{({ store }) => {
dispatchSpy = jest.spyOn(store, 'dispatch');
return <Foo />;
}}
</TestContext>
</DataProviderContext.Provider>
);
getByTestId = res.getByTestId;
});

const testElement = getByTestId('test');
await waitForDomChange({ container: testElement });

expect(dispatchSpy).toHaveBeenCalledWith(
showNotification('Youhou!', 'info', {
messageArgs: {},
undoable: false,
})
);
});

it('supports declarative onFailure side effects', async () => {
let dispatchSpy;
const dataProvider = jest.fn();
dataProvider.mockImplementationOnce(() =>
Promise.reject({ message: 'provider error' })
);

let getByTestId;
act(() => {
const res = render(
<DataProviderContext.Provider value={dataProvider}>
<TestContext>
{({ store }) => {
dispatchSpy = jest.spyOn(store, 'dispatch');
return (
<Query
type="GET_LIST"
resource="foo"
options={{
onFailure: {
notification: {
body: 'Damn!',
level: 'warning',
},
redirectTo: '/a_path',
refresh: true,
unselectAll: true,
},
}}
>
{({ loading, data, total }) => (
<div
data-testid="test"
className={
loading ? 'loading' : 'idle'
}
>
{loading ? 'no data' : total}
</div>
)}
</Query>
);
}}
</TestContext>
</DataProviderContext.Provider>
);
getByTestId = res.getByTestId;
});

const testElement = getByTestId('test');
await waitForDomChange({ container: testElement });

expect(dispatchSpy).toHaveBeenCalledWith(
showNotification('Damn!', 'warning', {
messageArgs: {},
undoable: false,
})
);
expect(dispatchSpy).toHaveBeenCalledWith(push('/a_path'));
expect(dispatchSpy).toHaveBeenCalledWith(refreshView());
expect(dispatchSpy).toHaveBeenCalledWith(setListSelectedIds('foo', []));
});

it('supports onFailure function for side effects', async () => {
let dispatchSpy;
const dataProvider = jest.fn();
dataProvider.mockImplementationOnce(() =>
Promise.reject({ message: 'provider error' })
);

const Foo = () => {
const notify = useNotify();
return (
<Query
type="GET_LIST"
resource="foo"
options={{
onFailure: () => {
notify('Damn!', 'warning');
},
}}
>
{({ loading, data, total }) => (
<div
data-testid="test"
className={loading ? 'loading' : 'idle'}
>
{loading ? 'no data' : total}
</div>
)}
</Query>
);
};
let getByTestId;
act(() => {
const res = render(
<DataProviderContext.Provider value={dataProvider}>
<TestContext>
{({ store }) => {
dispatchSpy = jest.spyOn(store, 'dispatch');
return <Foo />;
}}
</TestContext>
</DataProviderContext.Provider>
);
getByTestId = res.getByTestId;
});

const testElement = getByTestId('test');
await waitForDomChange({ container: testElement });

expect(dispatchSpy).toHaveBeenCalledWith(
showNotification('Damn!', 'warning', {
messageArgs: {},
undoable: false,
})
);
});
});
8 changes: 7 additions & 1 deletion packages/ra-core/src/dataProvider/Query.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ const Query: FunctionComponent<Props> = ({
resource,
payload,
options,
}) => children(useQuery({ type, resource, payload }, options));
}) =>
children(
useQuery(
{ type, resource, payload },
{ ...options, withDeclarativeSideEffectsSupport: true }
)
);

export default Query;
76 changes: 76 additions & 0 deletions packages/ra-core/src/dataProvider/useDataProviderWithDeclarativeSideEffects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import {
useNotify,
useRedirect,
useRefresh,
useUnselectAll,
} from '../sideEffect';
import useDataProvider, { DataProviderHookFunction } from './useDataProvider';
import { useCallback } from 'react';

/**
* This version of the useDataProvider hook ensure Query and Mutation components are still usable
* with side effects declared as objects.
*
* This is for backward compatibility only and will be removed in next major version.
*/
const useDataProviderWithDeclarativeSideEffects = (): DataProviderHookFunction => {
fzaninotto marked this conversation as resolved.
Show resolved Hide resolved
const dataProvider = useDataProvider();
const notify = useNotify();
const redirect = useRedirect();
const refresh = useRefresh();
const unselectAll = useUnselectAll();

return useCallback(
(type: string, resource: string, params: any, options: any = {}) => {
const convertToFunctionSideEffect = sideEffects => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can move this method at the top of the file or in another file, and write unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

No he can't, because the scope of the function must contain the side effects grabbed from hooks just above

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not possible to pass them as params?

if (!sideEffects || typeof sideEffects === 'function') {
return sideEffects;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can test null too. But it's not possible using typeof, because null has type 'object'`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reviewed

}

if (Object.keys(sideEffects).length === 0) {
return undefined;
}

const {
notification,
redirectTo,
refresh: needRefresh,
unselectAll: needUnselectAll,
} = sideEffects;

return () => {
if (notification) {
notify(
notification.body,
notification.level,
notification.messageArgs
);
}

if (redirectTo) {
redirect(redirectTo);
}

if (needRefresh) {
refresh();
}

if (needUnselectAll) {
unselectAll(resource);
}
};
};

const onSuccess = convertToFunctionSideEffect(options.onSuccess);
const onFailure = convertToFunctionSideEffect(options.onFailure);
return dataProvider(type, resource, params, {
...options,
onSuccess,
onFailure,
});
},
[dataProvider, notify, redirect, refresh, unselectAll]
);
};

export default useDataProviderWithDeclarativeSideEffects;
Loading