-
-
Notifications
You must be signed in to change notification settings - Fork 780
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
Conversation
tested v5 with/without new arch, it is not throwing this error |
Really!? so it's unnecessary in v5, feel free to close the PR if so. @gorhom |
@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) |
it doesn't work with newArch=true even on 5.0.0-alpha.10 and RN 0.74.1 @gorhom |
@ThusSpokeNomad @contactsimonwilson - please share a minimal reproducible example |
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>
);
};
{
"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 |
I've also hit this issue on latest v5 and RN 0.74, it seems to happen only when using |
There was a problem hiding this 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.
cc @gorhom re: @janicduplessis' comment - should we proceed with this pr? |
@janicduplessis I don't see a getRefNativeTag.web in the source code. |
Looks like it is only in the v5 branch, nvm then! |
confirmed it solved the issue in my project with RN0.74.2 + New Arch |
cc @gorhom - wdyt? |
@AndreiCalazans sorry i have been very busy with work lately,, just to confirm is this fix backward compatible with old arch ? |
@gorhom Yes. This proxy we included was a workaround for the new arch when it didn't have support for findNodeHandle - no longer required. |
thanks for confirming @AndreiCalazans , this fix should be out for the next releases of |
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.