Swipe tab-change causes oscillating race between delegated state and animation state

[solidfox]

Current behaviour

When releasing a swipe that initiates a tab change the tab view starts oscillating between snapping to the next and previous tab. It continues to oscillate some 4-10 times before settling on a random tab.

Expected behaviour

Tab view should snap to the tab that was swiped to without oscillating.

Code sample

Maybe later.

Screenshots (if applicable)

Maybe later.

What have you tried

The problem disappears when I remove this line in Pager.tsx.

I know that my state takes an unusually long time to re-render components after an update (~100ms). It would thus seem like we have a race condition where TabView assumes that the state it sees when forcing the update has the updated index. When this is not the case it starts snapping back to the previous tab, then the component gets updated with the new index and the oscillation is afoot.

Your Environment

software	version
ios or android	13.0
react-native	0.61.2
react-native-tab-view	2.10.0
react-native-gesture-handler	1.4.1
react-native-reanimated	1.3.0
node	12.11.1
npm or yarn	6.11.3

[solidfox]

I understand that the forceUpdate was added to allow the user to reject a tab change by not updating the state. I do agree that this would be sensible if it weren't for the fact that we cannot assume that the state will update synchronously or in time for the forced update.

Given the highly synchronous nature of the gesture animation I'd say a rejection behavior should be declared before starting the swipe, probably by simply declaring some tabs inactive or by allowing the user to return an index to snap to from the onIndexChange, thus guaranteeing that this decision is made synchronously.

With confirmation that either of those solutions are sensible I'll go ahead and write a PR for it.

[sgeier]

@solidfox Hi! I was wondering how you were able to build the module. I am trying to add in additional functionality that allows defining colors for each of the tabs.

I pass in to TabBar a simple color array of hex colors e.g. #123123.

<TabBar {...props} indicatorStyle={styles.indicator} style={styles.tabbar} tabStyle={styles.tab} tabsColorMap={[ this.tab1Color, this.tab2Color, this.tab3Color, this.tab4Color, this.tab5Color ]}/>

Now I wanted to pass through the tabsColorMap from TabBar to TabBarItem to TouchableItem and apply the color code there.

But I cannot test my changes as the module doesn't compile propertly. I ran npm install to load the dependencies. Then right after that it tries to run bob and fails with the following.

Can you advise to build it correctly, please?

bob build

ℹ Building target commonjs ℹ Cleaning up previous build at lib/commonjs ℹ Compiling 11 files in src with babel ✓ Wrote files to lib/commonjs ℹ Building target module ℹ Cleaning up previous build at lib/module ℹ Compiling 11 files in src with babel ✓ Wrote files to lib/module ℹ Building target typescript ℹ Cleaning up previous build at lib/typescript ℹ Generating type definitions with tsc ✖ Errors found when building definition files. ../../tsconfig.json(6,5): error TS5053: Option 'declaration' cannot be specified with option 'isolatedModules'. ../../tsconfig.json(10,5): error TS5053: Option 'emitDeclarationOnly' cannot be specified with option 'noEmit'.

[satya164]

we cannot assume that the state will update synchronously or in time for the forced update

We don't assume that state will update synchronously since setState is not synchronous, however we do need the state to be updated when the next update happens (i.e. when componentDidUpdate will be called).

By calling this.forceUpdate() we're just triggering an update, which React should batch with the setState due to the index change and cause a single update. Even if it isn't batched setState should be called before forceUpdate, so by the time the second update happens, it should be in sync anyway.

I might be misunderstanding how React batches updates. However many controlled components in React Native core are implemented this way, so I'm not sure if it's incorrect. So I'll need to see your code and repro to see if there's a problem that needs to be fixed. I can't accept a PR without first looking at the issue.

[solidfox]

That does seem sensible although it seems to be leaning heavy on implementation details. In this case, however, my state does not live within React. I’m using a redux-inspired architecture running on Reagent (a rather thick clojurescript wrapper on react) that I believe does its own batching so anything that relies on React batching would be brittle.

I guess it’s quite a corner case but anyone using ClojureScript will run the risk of running in to it since the norm is to not use React’s state for much of anything.

[solidfox]

Thanks to @satya164's insightful comments I looked closer at the batching and concluded that Reagent is doing its own batching (presumably a relic from before React had batching). I then found a way to flush Reagent's state after changing it by simply running (reagent/flush) and that fixed the problem.

I will maintain, however, that the current design is brittle and if possible to instead use a return value or pre-declared behavior that would make the timing requirements more clear and remove dependency on implementation details. But if the current design is consistent with other controlled components then that discussion has to happen at a higher level.

[satya164]

Even if I implemented a separate prop to prevent switching the tabs, this component is a controlled component and it needs to be in sync with the provided props. So if I don't take care of props being out of sync with what's displayed it will be buggy. It probably will be fine if you use this component directly, but integration with libraries such as React Navigation will break in unexpected ways if we don't guarantee about the component being in sync with its props.

React Native's built in components implement the same approach to achieve controlled components as well (check the code of TextInput), which is where I got the idea from. If I find a better way of guaranteeing that the component is never out of sync with it props, I'd do it. Ideas welcome.

I think it's a reasonable implementation because this library is made to be used with React, so we can rely on React's behaviour. If you use it with something other than React which implements its own behaviours and quirks, we wouldn't be able to guarantee it.

[solidfox]

One solution would be to add a different prop onIndexChangeWithReturn and if this prop is used instead of onIndexChange use the return value of that function as the effective index until new props come along and without forcing the update. But I agree that doing the same thing that the built in components do is probably best and if it's something people doing obscure things will have to work around anyway then why bloat the API.

The original onIndexChanged could do the same thing and just condition the forced update on whether an index was returned but that would be a breaking change for anyone inadvertently returning something from onIndexChanged.