software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
5.85k stars 954 forks source link

Fix destructuring `ref.props` on the web #1220

Closed hosseinmd closed 3 years ago

hosseinmd commented 3 years ago

Description

Fixes #1164.

It seems there is no way to get props from ref after react-native-web migrate to functional component, so I passed props from Handler component to gestureHandler class. Now that works well.

Test plan

import React from "react";
import { Text, View } from "react-native";
import {
  PanGestureHandler,
  TapGestureHandler,
} from "react-native-gesture-handler";

function App() {
  return (
    <>
      <Text>Log PanGestureHandler </Text>
      <View
        style={{
          backgroundColor: "yellow",
          overflow: "hidden",
        }}
      >
        <PanGestureHandler
          onGestureEvent={(gestureEvent) => {
            console.log({ gestureEvent });
          }}
        >
          <TapGestureHandler
            onHandlerStateChange={(stateEvent) => {
              console.log({ stateEvent });
            }}
          >
            <View
              style={{
                backgroundColor: "blue",
                height: 100,
                width: 200,
                margin: 20,
                alignItems: "center",
                justifyContent: "center",
              }}
            ></View>
          </TapGestureHandler>
        </PanGestureHandler>
      </View>
    </>
  );
}

export default App;
connectdotz commented 3 years ago

hey @hosseinmd thanks for submitting this PR, I tried it and found a problem: in createHandler.js the handler's props are set upon componentDidMount() but was never updated in componentDidUpdate() when the props changed, for example, a new onGestureEvent...

hosseinmd commented 3 years ago

hey @hosseinmd thanks for submitting this PR, I tried it and found a problem: in createHandler.js the handler's props are set upon componentDidMount() but was never updated in componentDidUpdate() when the props changed, for example, a new onGestureEvent...

Done

calumjames commented 3 years ago

When this is merged and tested, could it be released as a hotfix please? It stops gesture support on the latest stable version of React Native for Web. Thanks.

connectdotz commented 3 years ago

@hosseinmd still not quite working... looks like the _update method if (!deepEqual(this._config, newConfig)) will ignore some props like onGestureEvent... see filterConfig()

hosseinmd commented 3 years ago

@connectdotz Because of deepEqual checking, I just gave it the needed events in props, not the whole props.

connectdotz commented 3 years ago

@hosseinmd looks like we are heading down toward a more and more involved solutioin... what do you think if we pass the this.props in createHandler._attachGestureHandler as a ref instead? and in the componentDidUpdate() we just update the ref always this._propsRef.current = this.props. Then we don't need to touch any other methods to keep the change scope minimal...?

hosseinmd commented 3 years ago

Yes, but something is not right, _attachGestureHandler running just when viewTag change

      const viewTag = findNodeHandle(this._viewNode);
      if (this._viewTag !== viewTag) {
        this._attachGestureHandler(viewTag);
      }
connectdotz commented 3 years ago

you don't need to call _attachGestureHandler again in componentDidUpdate(), just update the ref directly should work:

componentDidUpdate() {
      const viewTag = findNodeHandle(this._viewNode);
      if (this._viewTag !== viewTag) {
        this._attachGestureHandler(viewTag);
      }
      this._propsRef.current = this.props;
      this._update();
    }
hosseinmd commented 3 years ago

All right, I understand that now.

hosseinmd commented 3 years ago

Off topic, examples have old versions of modules, perhaps should be updated to new versions.

chadhobson commented 3 years ago

Thanks to both of you. I can also confirm this fixes the issue.

Here's a patch-package to get folks back on track until this gets merged.

react-native-gesture-handler+1.8.0.patch.zip

sekonia commented 3 years ago

I ran into the same problems... thank you @hosseinmd and @connectdotz for fixing this and @chadhobson for the patch. Looking forward to a release update. 🎉

hosseinmd commented 3 years ago

This PR lack a proper test plan. Could you write some small repro example on which we can test it?

I left some comments inline.

Thanks for your review, for test this we should update the example of this repo to the new version of react-native-web.

jakub-gonet commented 3 years ago

I think this is its own issue, our example projects are terribly outdated. I'd be content with a link to the repo using rn for web & rngh with some small code example.

hosseinmd commented 3 years ago

I think this is its own issue, our example projects are terribly outdated. I'd be content with a link to the repo using rn for web & rngh with some small code example.

Ok, I will do it as soon as possible.

hosseinmd commented 3 years ago

@jakub-gonet Please test this repo https://github.com/hosseinmd/test-gesture-handler

hosseinmd commented 3 years ago

I have done what you say. I added two examples to this repo https://github.com/hosseinmd/test-gesture-handler, but something is wrong yet because useAnimatedGestureHandler doesn't work.

jakub-gonet commented 3 years ago

Thanks, I'll try to look into it in the next few days.

jakub-gonet commented 3 years ago

Ok, I checked it and seems like the error you were experiencing was fixed in react-native-reanimated#1402 (yet unreleased). I applied this patch locally and seems like "Log PanGestureHandler" and "Bouncing" example work. The swipeable list doesn't but it also isn't crashing with ref.props error.

I also checked nested handlers and it works alright. I'll update the PR description with the example you wrote and I think we're good to go.

Please reply if you want to test it more or we can merge this.

hosseinmd commented 3 years ago

I tested it in my project and it works. Except for useAnimatedGestureHandler, I think it's not related to this error, I will work on it in another pr. please merge this. Thank you.

jakub-gonet commented 3 years ago

Sure, thanks for your contribution!