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

Strict mode compat (web) by removing findNodeHandle #4802

Closed
natew opened this issue Jul 24, 2023 · 18 comments
Closed

Strict mode compat (web) by removing findNodeHandle #4802

natew opened this issue Jul 24, 2023 · 18 comments
Labels
Missing repro This issue need minimum repro scenario Platform: Web This issue is specific to web

Comments

@natew
Copy link

natew commented Jul 24, 2023

Description

On web you get errors:

Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of AnimatedComponent(View) which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here

This is due to findNodeHandle which according to react-native-web is deprecated:

https://github.com/necolas/react-native-web/blob/ef95fc0c2b40cc0d3743df4b56485a16a8442e5f/packages/react-native-web/src/exports/findNodeHandle/index.js#L17

Would be nice to remove this usage. I can help with this with just some light pointers in terms of what thats needed for now.

Steps to reproduce

Use any animated component on web, sorry for no repro but easy to trigger.

Snack or a link to a repository

https://snack.expo.io/

Reanimated version

3.3.0

React Native version

0.72.0

Platforms

Web

JavaScript runtime

None

Workflow

None

Architecture

None

Build type

None

Device

None

Device model

No response

Acknowledgements

Yes

@natew natew added the Needs review Issue is ready to be reviewed by a maintainer label Jul 24, 2023
@github-actions github-actions bot added Platform: Web This issue is specific to web Missing repro This issue need minimum repro scenario labels Jul 24, 2023
@github-actions
Copy link

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

@tomekzaw tomekzaw removed the Needs review Issue is ready to be reviewed by a maintainer label Jul 24, 2023
@tomekzaw
Copy link
Member

Hey @natew, thanks for pointing out this. Someone from our team will take a look at it soon.

@m-bert
Copy link
Contributor

m-bert commented Aug 7, 2023

Hi @natew! This PR should remove all calls of findNodeHandle on web. Could you please test it and confirm that it solves this issue?

@natew
Copy link
Author

natew commented Aug 12, 2023

It does, thank you a bunch, will look forward to the release with it. I'm not positive but my guess is our drop in performance/lighthouse recently on the site was due to this causing React to de-opt concurrent rendering which makes our main thread work go up quite a bit. Testing that next..

@m-bert m-bert closed this as completed Aug 22, 2023
@asenmitrev
Copy link

asenmitrev commented Oct 23, 2023

@m-bert Seems findNodeHandle is still used in https://github.com/software-mansion/react-native-reanimated/blob/main/src/createAnimatedComponent/JSPropUpdater.ts#L45 , as well as some other places. Is this an issue with my setup that JSPropUpdater is used at all?

Edit: After running the NextExample from the reanimated main branch, and adding React.StrictMode, the warning still appears.

@m-bert
Copy link
Contributor

m-bert commented Oct 23, 2023

Hi @asenmitrev! It is possible that something has changed and now JSPropUpdater is used on web. I'll look at it and get back to you.

@m-bert
Copy link
Contributor

m-bert commented Oct 26, 2023

Hi @asenmitrev! Could you please check if this PR works and warnings are gone?

@asenmitrev
Copy link

@m-bert checked on NextExample in the PR branch and seems the warning is gone. All seems good, thank you!

github-merge-queue bot pushed a commit that referenced this issue Oct 26, 2023
## Summary

See [this
issue](#4802)
for context.

It seems that `findNodeHandle` was still used on web in `JSPropUpdater`.
This PR removes its usage.

## Known issue

It seems that in `findNodeHandle(ref) === ref` (used in
`createAnimatedComponent` ) isn't true in all cases (it can happen for
example with `SVG`)

## Test plan

Tested on update props performance example inside `<React.StrictMode>`
bartlomiejbloniarz pushed a commit that referenced this issue Oct 27, 2023
## Summary

See [this
issue](#4802)
for context.

It seems that `findNodeHandle` was still used on web in `JSPropUpdater`.
This PR removes its usage.

## Known issue

It seems that in `findNodeHandle(ref) === ref` (used in
`createAnimatedComponent` ) isn't true in all cases (it can happen for
example with `SVG`)

## Test plan

Tested on update props performance example inside `<React.StrictMode>`
@apetta
Copy link

apetta commented Nov 19, 2023

Still seem to be getting this on 3.5.4

@tomekzaw
Copy link
Member

@apetta Yes, because this PR was not released yet (will be released in 3.6.0). For now, you can use react-native-reanimated@nightly.

@tomekzaw
Copy link
Member

@apetta FYI we have just published react-native-reanimated@3.6.0.

@YaoHuiJi
Copy link

YaoHuiJi commented Feb 8, 2024

hi, @tomekzaw, this issue still exists on iOS with react-native-reanimated 3.6.1

Warning: findNodeHandle is deprecated in StrictMode. findNodeHandle was passed an instance of AnimatedComponent(ScrollView) which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://react.dev/link/strict-mode-find-node in RCTScrollView (created by ScrollView) in ScrollView (created by ScrollView) in ScrollView (created by AnimatedComponent(ScrollView)) in AnimatedComponent(ScrollView)

@m-bert
Copy link
Contributor

m-bert commented Feb 8, 2024

Hi @YaoHuiJi! I've just went through our code and I think I've managed to find all of the remaining calls. Could you please take a look at #5654 and confirm that it solves this problem?

@YaoHuiJi
Copy link

YaoHuiJi commented Feb 8, 2024

Hi @m-bert , thank you for your reply, I'm not familiar with the source code of react-native-reanimated, so I don't know if I'm right, but I took a quick look at #5654, and it looks like it's just for the web. Take for example the following statement:
viewTag = IS_WEB ? this._component : findNodeHandle(options? .setNativeProps ? this : node);

Does this mean findNodeHandle is still being called for platforms other than the web?

@m-bert
Copy link
Contributor

m-bert commented Feb 8, 2024

Exactly, this PR removes those call from web. This was the original problem in the issue.

@YaoHuiJi
Copy link

YaoHuiJi commented Feb 8, 2024

Exactly, this PR removes those call from web. This was the original problem in the issue.

Hi @m-bert , I found a merged pr Deprecate findDOMNode in StrictMode, according to the reply, it looks like all usages should be deprecated, maybe this issue should be applied to all platforms, not only web?

@tomekzaw
Copy link
Member

tomekzaw commented Feb 8, 2024

@YaoHuiJi Would be great to do that but findNodeHandle is the only known way to obtain view tag in React Native which we need on the native side to apply animated styles, register for events etc.

@YaoHuiJi
Copy link

YaoHuiJi commented Feb 8, 2024

@YaoHuiJi Would be great to do that but findNodeHandle is the only known way to obtain view tag in React Native which we need on the native side to apply animated styles, register for events etc.

@tomekzaw I understand now, thank you for your explanation💗

github-merge-queue bot pushed a commit that referenced this issue Feb 9, 2024
## Summary

As mentioned in [this
comment](#4802 (comment)),
some of `findNodeHandle` calls are still present on web. This PR aims to
remove them in order to get rid of errors in `StrictMode`.

## Test plan

Tested on examples (mainly with scroll, like "ScrollViewExample" or
"ScrollEventExample")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing repro This issue need minimum repro scenario Platform: Web This issue is specific to web
Projects
None yet
Development

No branches or pull requests

6 participants