react-native-bottom-sheet icon indicating copy to clipboard operation
react-native-bottom-sheet copied to clipboard

fix: replace getRefNativeTag with findNodeHandle

Open AndreiCalazans opened this issue 1 year ago • 8 comments
trafficstars

Please provide enough information so that others can review your pull request:

Motivation

Trying to fix this issue when new arch is enabled: https://github.com/gorhom/react-native-bottom-sheet/issues/1726

Current getRefNativeTag causes this issue with new architecture:

By reverting to findNodeHandle it works:

https://github.com/gorhom/react-native-bottom-sheet/assets/20777666/ea29dda2-987a-47a1-a4c3-6a569a8916f6

We had introduced getRefNativeTag because experimental new architecture last year did not support using findNodeHandle. This is no longer true with the proxy renderer they created.

This was tested with and without new architecture enabled on iOS.

AndreiCalazans avatar May 02 '24 20:05 AndreiCalazans

tested v5 with/without new arch, it is not throwing this error

gorhom avatar May 19 '24 16:05 gorhom

Really!? so it's unnecessary in v5, feel free to close the PR if so. @gorhom

AndreiCalazans avatar May 20 '24 13:05 AndreiCalazans

@gorhom can confirm v5 is not entirely working on new arch. I'm getting this after switching to new arch (also tried updating from alpha 9 to 10) image

techfoundrynz avatar May 22 '24 12:05 techfoundrynz

it doesn't work with newArch=true even on 5.0.0-alpha.10 and RN 0.74.1 @gorhom image

0xFA11 avatar May 31 '24 22:05 0xFA11

@ThusSpokeNomad @contactsimonwilson - please share a minimal reproducible example

brentvatne avatar Jun 03 '24 21:06 brentvatne

@brentvatne

index.tsx

import { useState, useEffect } from "react";
import { Text } from "react-native";
import { GestureHandlerRootView } from "react-native-gesture-handler";
import WebView from "react-native-webview";
import BottomSheet, { BottomSheetScrollView } from "@gorhom/bottom-sheet";

export default () => {
    const [lipsum, setLipsum] = useState<string>();
    useEffect(() => {
        void (async () => {
            const lipsumJSON = await (await fetch("https://www.lipsum.com/feed/json")).json();
            setLipsum(lipsumJSON.feed.lipsum);
        })();
    }, []);
    return (
        <GestureHandlerRootView>
            <WebView source={{ uri: "https://www.lipsum.com/feed" }} />
            <BottomSheet snapPoints={["50%", "100%"]}>
                <BottomSheetScrollView>
                    <Text>{lipsum}</Text>
                </BottomSheetScrollView>
            </BottomSheet>
        </GestureHandlerRootView>
    );
};

package.json

{
    "name": "minrepro",
    "main": "expo-router/entry",
    "scripts": {
        "android": "expo run:android",
        "ios": "expo run:ios"
    },
    "dependencies": {
        "@gorhom/bottom-sheet": "^5.0.0-alpha.10",
        "expo": "^51.0.9",
        "expo-build-properties": "0.12.1",
        "expo-router": "3.5.15",
        "react": "18.2.0",
        "react-native": "0.74.1",
        "react-native-gesture-handler": "2.16.1",
        "react-native-reanimated": "3.10.1",
        "react-native-safe-area-context": "4.10.1",
        "react-native-webview": "13.8.6"
    },
    "devDependencies": {
        "@types/react": "18.2.79",
        "typescript": "5.3.3"
    }
}

also, interestingly, if you delete useState hook then it works fine (and useEffect could just console.log for testing).

0xFA11 avatar Jun 04 '24 01:06 0xFA11

I've also hit this issue on latest v5 and RN 0.74, it seems to happen only when using BottomSheetScrollView. This patch fixes it for me.

janicduplessis avatar Jun 22 '24 04:06 janicduplessis

cc @gorhom re: @janicduplessis' comment - should we proceed with this pr?

brentvatne avatar Jun 25 '24 23:06 brentvatne

@janicduplessis I don't see a getRefNativeTag.web in the source code.

AndreiCalazans avatar Jul 05 '24 14:07 AndreiCalazans

Looks like it is only in the v5 branch, nvm then!

janicduplessis avatar Jul 05 '24 14:07 janicduplessis

confirmed it solved the issue in my project with RN0.74.2 + New Arch

craftzdog avatar Jul 10 '24 06:07 craftzdog

cc @gorhom - wdyt?

brentvatne avatar Jul 22 '24 22:07 brentvatne

@AndreiCalazans sorry i have been very busy with work lately,, just to confirm is this fix backward compatible with old arch ?

gorhom avatar Jul 28 '24 20:07 gorhom

@gorhom Yes. This proxy we included was a workaround for the new arch when it didn't have support for findNodeHandle - no longer required.

AndreiCalazans avatar Jul 28 '24 20:07 AndreiCalazans

thanks for confirming @AndreiCalazans , this fix should be out for the next releases of v4 and v5

gorhom avatar Jul 29 '24 06:07 gorhom