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

Fix type error when queryOptions enable is set to false #7987

Merged
merged 5 commits into from
Sep 23, 2022

Conversation

yanchesky
Copy link
Contributor

Fixes #7986

@djhi
Copy link
Contributor

djhi commented Jul 19, 2022

Thanks for the error report and the PR! Would you mind trying to add a unit test for this case?

Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

packages/ra-core/src/controller/list/useListController.ts Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/list/datagrid/Datagrid.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

@djhi djhi added this to the 4.2.4 milestone Jul 21, 2022
@yanchesky
Copy link
Contributor Author

Fixing failed unit test might be cumbersome 🤔. I have run it on the local machine and out of 10 tests on unchanged code:

  • 4 times all tests passed
  • 3 times "should notify if passed an error with a message that makes the authProvider throw" test failed
  • 2 times "should take only last change in case of a burst of changes (case of inputs being currently edited)" test failed
  • 1 time both 👆 failed

@fzaninotto fzaninotto removed this from the 4.2.4 milestone Jul 22, 2022
@yanchesky
Copy link
Contributor Author

I couldn't find the exact reason why the test should take only last change in case of a burst of changes (case of inputs being currently edited) sometimes fails. I've run this test only and it never failed. I suspect that it may be related to react-router-dom because right before the test fails, the error is raised An update to HistoryRouter inside a test was not wrapped in act. However, this error occurs even if the test passes so I'm not sure about this.
However, knowing that I've come up with 2 solutions that make this test suite work. (By work I mean 10 test runs without any fail)

  1. Create history and pass it to the CoreAdminContext component.
  2. pass disableSyncWithLocation prop to the ListController component.

If that is ok with you I will make a commit fixing it.

Snapshot of the mock requests that cause the test to fail:

"calls": Array [
    Array [
      "posts.listParams",
      Object {
        "filter": Object {},
        "order": "ASC",
        "page": 1,
        "perPage": 10,
        "sort": "id",
      },
    ],
    Array [
      "posts.listParams",
      Object {
        "filter": Object {
          "q": "hello",
        },
        "order": "ASC",
        "page": 1,
        "perPage": 10,
        "sort": "id",
      },
    ],
  ],

@ITopGun
Copy link

ITopGun commented Jul 27, 2022

Hi

@WiXSL
Copy link
Contributor

WiXSL commented Sep 7, 2022

Could you resolve the conflict?

@@ -109,6 +109,7 @@ export const useListController = <RecordType extends RaRecord = any>(
useEffect(() => {
if (
query.page <= 0 ||
data == null ||
Copy link
Contributor

@slax57 slax57 Sep 8, 2022

Choose a reason for hiding this comment

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

Tests might be failing because you trigger setPage(1) even though you are already on page 1, which causes store to update and might trigger a re-render.

To me, you should rather move data == null alongside with data?.length === 0 in the line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info. I will check it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it as you suggested and seems it's working now. However on my machine, some tests are failing, but they are failing on the master branch as well.
unit test that fail: "should apply validation to both itself and its inner inputs"
e2e tests that fail: "should validate ArrayInput", and sometimes "should have tabbable menu items"

@fzaninotto fzaninotto merged commit 8015302 into marmelab:master Sep 23, 2022
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 4.3.4 milestone Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List props queryOptions enabled set to false causes type error.
6 participants