Pagination issue when pagination on server

[sarathdr]

Issue

[jbetancur]

Thanks for taking a stab at fixing this.

I don't think you even need to re-calculate. Just detect if the paginationTotalRows changed then check the currentPage is > 1 and there are 0 records in the data.length or calculatedRows.length. then just move back a page

  if (pagination && paginationServer && ) {
    useDidUpdateEffect(() => {
      if (calculatedRows.length === 0 && currentPage > 1) {
        handleChangePage(currentPage - 1);
      }
    }, [paginationTotalRows]);
  }

I would also use useDidUpdateEffect since it doesn't fire on initial render

[jbetancur]

  if (pagination && paginationServer) {
    useDidUpdateEffect(() => {
      if (calculatedRows.length === 0 && currentPage > 1) {
        handleChangePage(currentPage - 1);
      }
    }, [paginationTotalRows]);
  }

[sarathdr]

  if (pagination && paginationServer && ) {
    useDidUpdateEffect(() => {
      if (calculatedRows.length === 0 && currentPage > 1) {
        handleChangePage(currentPage - 1);
      }
    }, [paginationTotalRows]);
  }

calculatedRows.length this will be > 0 since server should fetch the data for the last page?
only the total changes?

[jbetancur]

calculatedRows.length when using paginationServer should always be the same as data.length. It would probably be less confusing to just use data.length.

Since you are "server-side" paging the calculatedRows.length (or data.length) will only be whatever the total length of the data loaded from you api is. So, if you're paging for 10 records per page the length is 10 even though you may have hundreds or more of records.

What needs to be done is when the data.length reaches 0 then we trigger the pagination back one page. i.e. you delete the last record on a page (data.length is 0)

This is why paginationTotalRows matters when using paginationServer and why it should be set and kept updated to whatever your pagination api returns as the total rows so that RDT can calculate what the total rows actually are instead of from data.length. If you are in fact deleting a record your API should return the updated total rows so that the pagination counter can recalculate.

All of this works differently in client-side pagination since we have the entire dataset loaded which i why the code uses calculatedRows.length for those calculations (it uses Array.slice to give you the effect of pagination)

[jbetancur]

 if (pagination && paginationServer) {
    useDidUpdateEffect(() => {
      if (data.length === 0 && currentPage > 1) {
        handleChangePage(currentPage - 1);
      }
    }, [paginationTotalRows]);
  }

[sarathdr]

 if (pagination && paginationServer) {
    useDidUpdateEffect(() => {
      if (data.length === 0 && currentPage > 1) {
        handleChangePage(currentPage - 1);
      }
    }, [paginationTotalRows]);
  }

This is not working you can rerun the test that I have written. I think the use cases are

• Suppose you have 22 records - 3 pages ( 10 per page )

• Page 1 - server loads -> 10 records - page = 1

• Page 2 - serverLoads -> 10 records - page = 2

• Page 3 - serverLoads -> 2 record - page = 3

• Delete last item - server re-loads (last page only ie page 3) -> 1 record, page = 3

• Delete last item (21 records) - server re-loads (last page only ie page 2) 10 records, page = 2

• In the last use case data.length = 10 so if (data.length === 0 && currentPage > 1) will not recalculate position even if total has been changed

• Otherwise, ( as per your logic ) I have to mutate local state to remove items from current page data rather than reloading it from the server which is inconsistent?

[janhartmann]

Please see https://reactjs.org/docs/hooks-overview.html#rules-of-hooks

Only call Hooks at the top level. Don’t call Hooks inside loops, conditions, or nested functions.

[jbetancur]

@janhartmann - I forgot about that and totally agree. I think we need to re-think the design/implementation of useEffect hooks in DataTable. Also, we need to move these hooks into their own files/logic.

[sarathdr]

I thought about it as well, @jbetancur need to clean up that

[jbetancur]

#335

Empty right now, but a placeholder for refactor work

[jbetancur]

Strange thing is the hooks linter does not catch it

[jbetancur]

I just mocked up an actual delete against a live API and it seems to be working. @sarathdr your logic solves the issue. Just need to get that test working. I'll have a look

  // when server paginationServer go back one page if the last record is removed
  useDidUpdateEffect(() => {
    if (pagination && paginationServer && data.length === 0 && currentPage > 1) {
      handleChangePage(currentPage - 1);
    }
  }, [paginationTotalRows]);

[jbetancur]

Actually, if you want to just remove the tests/snapshot updates I can merge this in. I can add the test in the refactor I am doing

[janhartmann]

Remember to put the “pagination” in the dependency array. Good rule is that all used variables within the hook needs to go into the dependency array, so if any of these change it will re-compute.

[janhartmann]

Or not sure if needed in this case

[sarathdr]

Actually, if you want to just remove the tests/snapshot updates I can merge this in. I can add the test in the refactor I am doing

OK I will push after that

[codecov]

Codecov Report

Merging #333 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   99.76%   99.77%   +<.01%     
==========================================
  Files          40       40              
  Lines         432      439       +7     
  Branches       96       99       +3     
==========================================
+ Hits          431      438       +7     
  Misses          1        1

Impacted Files	Coverage Δ
src/DataTable/DataTable.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0d8b40...918b54d. Read the comment docs.

[jbetancur]

I created a codesandbox of what I am testing locally. It will serve as a validation once 4.0.1 is cut

https://codesandbox.io/s/react-data-table-server-side-pagination-delete-xmdju

1. Nav to last page
2. delete both records
3. should nav to page 1

[jbetancur]

@janhartmann not needed in this case. While it is good practice in general, this sort of update is specific to paginationTotalRows changing (ie, a remote delete where the API returns some different row count) we don't want the page check to fire if say pagination is togged to false

[jbetancur]

@sarathdr Make sure you rebase from master as I just made some ordering changes to useEffect

[sarathdr]

@sarathdr Make sure you rebase from master as I just made some ordering changes to useEffect

The pull request is closed, has this been merged ? Do you want me to re-open after rebase?