getResetPageDeps Causes Multiple Effects

[mendelk]

(Using v7.0.0-rc.1)

I'm not sure this qualifies as a bug, but I think it's surprising behavior.

When using pagination, and changing the sort, this resets the pagination (via getResetPageDeps).

If I have a useEffect that depends on pageIndex and sortBy pulled from the instance state, that effect gets run twice when sortBy changes, once with the current pageIndex, and then again with the reset pageIndex.

I'd argue that this is not what most people want or expect, because the main use-case for this is making an API call. The current behavior results in two API calls, one right after the other. Instead, this should be once state update, with one effect called.

I forked the controlled pagination CodeSandBox to illustrate: fork. As you can see, it logs twice if you're not on the first page.

Thanks for this wonderful code!

[tannerlinsley]

Hmmm. Yeah, I can see what you mean. This is an interesting conundrum. Currently, internal effects are "watching" the table state (and applying the reset deps) to detect when certain parts of the state should be "auto" reset. If we were to change this behavior to be closer to where the actual change happens, it would essentially need to be a state reducer plugin (which I have have debated adding before). Essentially it would need to detect in the pagination reducer whether or not certain deps have changed from the previous state (or simply detect actions), and then apply the page reset in the same reducer call to avoid what you are seeing.

Doing this would probably create more work for users that want to alter the auto-reset deps, since they would then have to fiddle with state reducers and manually detect actions or before/after changes in the reducer. However, assuming that was possible and implemented well, it would allow users to just continue using an effect to watch the state and react appropriately.

What are your thoughts? I'm all on board to make changes for this behavior, as long as we can find a really good API design that will work for across the board. While I wait for your input, I'm going to investigate what it would take to convert the necessity of getReset____Deps into something more reducer-like.

[mendelk]

@tannerlinsley I'm not super familiar with this code-base, but looking deeper, I see what you mean. I figured there must've been a good reason for it.

I'm just floating ideas here, but if we limited the argument to getResetPageDeps to simply the state (and not the entire instance), then perhaps we can implement some sort of middleware around the main reducer, while keeping the current API exactly the same (minus the limitation to deps in state). Not sure this would work though.

Still 🤔 ...

[tannerlinsley]

That could work. So far, I've moved the reducer handlers to be registered to a hook, thus making them aware of the instance. The problem I'm running into now is that most of the dependencies for auto-resetting state are not just in the state itself, but also depend on options passed to the table that may change, or in most cases, if the data option changes from render to render. Unfortunately, the reducer is really only active when an action happens, and not when anything outside of the state changes.

[tannerlinsley]

Forgive me if I'm digressing here, but it seems this situation could technically be solved with a debounce, no?

[mendelk]

Forgive me if I'm digressing here, but it seems this situation could technically be solved with a debounce, no?

Yes! And that's exactly how I intend to solve it if no good solution presents itself here (or at least not in time).

I figured I'd bring it up because I'm sure I won't be the only one to run into this. Also, a minor issue with debouncing is that it will cause a (minor) delay in every other server fetch. Trade-offs, as always 😄

[tannerlinsley]

Too bad there isn't a way to like... debounce or batch the action/effect/action sequence into a single fluid thing.

[tannerlinsley]

Let's chat for a minute about the auto-reset flow.

• Is it necessary?
• Is there a better way to compose it that isn't a side-effect?
• What are the most basic forms of the dependencies of these resets?
• Do we ship with defaults?
  • If so, how can users easily remove or extend the defaults?
  • If not, then how do we make them easily available to the user?
• What's the majority use case for auto-reset?

[mendelk]

(I need to get onto something else that came up, but I hope to think more about this over the weekend.)

My general thoughts are that if you're using pagination, in the vast majority of cases it will need to be reset on basically any change.

For example, if we change a filter, there's 0 cases I can think of where the user would still like to remain on page 3, for example. (In fact, there may no longer be a page 3!) And the same for changing the page size, etc.

Since I think for any sufficiently advanced use-case (= server-rendered data) this will become an issue, it's worth addressing in the core model.

IMHO, solving this in a way that isn't possible to extend (basically having it reset on any change no matter what), would still be an improvement on the behavior today.

Just my 2c thinking about this right now. But I'd like to take more time to delve in.

[tannerlinsley]

That does bring up one very good use case: Editable Data. We even have an example for this in the repo already too, where you you have to suppress this automatic behavior when you edit a cell, since the data reference changes, pagination thinks it needs to reset and BAM, you're in a weird UX state now.

Historically, there were props in React Table like resetPageOnDataChange={false}, but as you can imagine, this is very one-dimensional. With that kind of option, one would assume it's ONLY tied to data changing and ONLY responsible for resetting the page index.

I guess we could bring something like that back and have it live in the reducer (but not be extensible). Then just instruct users to make their own useEffects for doing custom stuff outside of it (that would still result in a double-action, but hey, it's custom).

We could even call it autoResetPageIndex, and default it to false. That way it represents the concept as a whole and doesn't open up too wide as an API.

So you could do something like this:

const [data, setData] = React.useState([])
const skipPageResetRef = React.useRef()

const updateData = newData => {
  // When data gets updated with this function, disable the
  // page from resetting
  skipPageResetRef.current = true

  setData(newData)
}

React.useEffect(() => {
  skipPageResetRef.current = false
})

useTable({
  data,
  autoResetPageIndex: !skipPageResetRef.current,
})

Then additionally, if you wanted to extend or replace the autoReset, you could do this:

import React from 'react'
import { useTable, actions } from 'react-table'

function Table({ data }) {
  const { dispatch } = useTable({
    data,
    autoResetPageIndex: false,
  })

  React.useEffect(() => {
    dispatch({ type: actions.resetPage })
  }, [dispatch, data])
}

Thoughts?

[Ksoso]

To solve the issue with auto resetting, I used React key property which you can set on every component. I'm setting in useTable() hook given properties like:

 getResetPageDeps: false,
 getResetSelectedRowPathsDeps: false,

because I'm using hook for pagination and row selection. When I'm sure that my component will get new data, I'm generating new key which i'm using until next data change. With this approach my table component will always be recreated when it's needed and inner state will get reset.

Moreover, I had big problems to integrate react-table into my current application because my single source of truth is Redux state and with Redux comes immutability. Data table is a center of application and many other components can change rows data in it or display data from it. By disabling resetting of state, custom reducer option and exposing useTable() instance methods by forwardRef I can control what i need in table created by react-table hooks. I know that my solution looks like one big hack but to be honest react-table is already top of the top in customization category and already implemented solution with ..Deps property feels natural to me when using React hooks.

[tannerlinsley]

That's good to hear. I am pretty sure you'll be able to continue to do basically the same thing with the new options.

So instead of getResetPageDeps, you'll just set autoResetPage: false and continue to imperatively update the table state as you need.

[tannerlinsley]

I think the best thing to do for now is to move forward with the auto-reset side-effects. They may cause multiple rerenders in rapid succession, but for that, I am actually offering a built-in async debouncer hook. You can check out an example here: https://github.com/tannerlinsley/react-table/blob/master/docs/faq.md#how-can-i-debounce-rapid-table-state-changes

As for controlling the auto-reset functionality, all of the getReset___Deps options have been removed in favor of the classic boolean to disable the functionality. More info on that here: https://github.com/tannerlinsley/react-table/blob/master/docs/faq.md#how-do-i-stop-my-table-state-from-automatically-resetting-when-my-data-changes

There are a few other changes you may want to note, available in the changelog: https://github.com/tannerlinsley/react-table/blob/master/CHANGELOG.md#700-rc2