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

separate test and default data providers #10001

Merged

Conversation

Nilegfx
Copy link
Contributor

@Nilegfx Nilegfx commented Jul 13, 2024

Problem

defaultDataProvider behaviour changed in V5. the change was intended to just support the tests. this affect the end users if they use the exported defaultDataProvider in their implementation which means one extra migration step for them and one extra migration documentation entry point react-admin team has to add.

Solution

Implement testing-related changes for the defaultDataProvider in the right place testDataProvider and revert the defaultDataProvider to its original behaviour (which is dummy responses but at least it doesn't throw)

Struggles/Challenges

I implemented the refactoring in 30 seconds, then I spent almost half an hour trying to make the defaultDataProvider work with the DataProvider type but I failed. I would appreciate if one of the core members can explain to me what I miss here or how to correctly type the defaultDataProvider.
you can reproduce the issue by just changing export const defaultDataProvider = { to export const defaultDataProvider:DataProvider = { (and of course import the DataProvider type.

I used a workaround export const defaultDataProvider: DataProvider = {} as DataProvider but I don't like this. I am still interested to understand this TypeError (I used AI but it couldn't give me logical answer)

How To Test

giving that this change was intended to keep everything as the same, I assumed that the current tests should be enough for this simple and small refactoring

Additional Checks

  • The PR targets master for a bugfix, or next for a feature
  • The PR includes unit tests (check the above How To Test section)
  • The PR includes one or several stories (check the above How To Test section)
  • The documentation is up to date

Also, please make sure to read the contributing guidelines.

@adguernier
Copy link
Contributor

Hi @Nilegfx and thanks for your contribution 👍

We used to type result as any in our tests and stories (e.g: getOne: () => Promise.resolve({ data: null } as any),. But to avoid using any, I'd type the defaultDataProvider as follows:

import {
    CreateResult,
    DataProvider,
    DeleteResult,
    GetOneResult,
    UpdateResult,
} from '../types';

export const defaultDataProvider: DataProvider = {
    create: () => Promise.resolve<CreateResult>({ data: null }),
    delete: () => Promise.resolve<DeleteResult>({ data: null }),
    deleteMany: () => Promise.resolve({ data: [] }),
    getList: () => Promise.resolve({ data: [], total: 0 }),
    getMany: () => Promise.resolve({ data: [] }),
    getManyReference: () => Promise.resolve({ data: [], total: 0 }),
    getOne: () => Promise.resolve<GetOneResult>({ data: null }),
    update: () => Promise.resolve<UpdateResult>({ data: null }),
    updateMany: () => Promise.resolve({ data: [] }),
};

What do you think about that @djhi ?

getList: () => Promise.resolve({ data: [], total: 0 }),
getMany: () => Promise.resolve({ data: [] }),
getManyReference: () => Promise.resolve({ data: [], total: 0 }),
getOne: () => Promise.resolve({ data: { id: 'o' } }),
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why { data: { id: 'o' } } instead of { data: null } like the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that was committed by mistake, I will remove it.

@adguernier
Copy link
Contributor

Awaiting further approval
Thanks again 👍

@djhi djhi added this to the 5.0.5 milestone Jul 19, 2024
@djhi djhi merged commit 9445f99 into marmelab:master Jul 19, 2024
13 checks passed
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

Successfully merging this pull request may close these issues.

3 participants