Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: replace getRefNativeTag with findNodeHandle #1823

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

AndreiCalazans
Copy link
Contributor

@AndreiCalazans AndreiCalazans commented May 2, 2024

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

Motivation

Trying to fix this issue when new arch is enabled: #1726

Current getRefNativeTag causes this issue with new architecture:

By reverting to findNodeHandle it works:

fabric-enabled-ios.mp4

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.

@gorhom
Copy link
Owner

gorhom commented May 19, 2024

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

@AndreiCalazans
Copy link
Contributor Author

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

@contactsimonwilson
Copy link

@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

@0xFA11
Copy link

0xFA11 commented May 31, 2024

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

@brentvatne
Copy link

@ThusSpokeNomad @contactsimonwilson - please share a minimal reproducible example

@0xFA11
Copy link

0xFA11 commented Jun 4, 2024

@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).

@janicduplessis
Copy link
Contributor

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.

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably also want to delete the getRefNativeTag.web.ts file.

@brentvatne
Copy link

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

@AndreiCalazans
Copy link
Contributor Author

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

@janicduplessis
Copy link
Contributor

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

@craftzdog
Copy link

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

@brentvatne
Copy link

cc @gorhom - wdyt?

@gorhom
Copy link
Owner

gorhom commented Jul 28, 2024

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

@gorhom gorhom self-assigned this Jul 28, 2024
@gorhom gorhom added v4 Written in Reanimated v2 v5 labels Jul 28, 2024
@AndreiCalazans
Copy link
Contributor Author

AndreiCalazans commented Jul 28, 2024

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

@gorhom gorhom merged commit b906f5e into gorhom:master Jul 29, 2024
1 check failed
@gorhom
Copy link
Owner

gorhom commented Jul 29, 2024

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

This was referenced Sep 8, 2024
This was referenced Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v4 Written in Reanimated v2 v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants