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

setNativeProps with style is incompatible with also updating style prop itself #1987

Closed
comp615 opened this issue Apr 12, 2021 · 6 comments
Closed

Comments

@comp615
Copy link
Contributor

comp615 commented Apr 12, 2021

Knocking this out quick before a meeting, but will try to understand more tonight.

The problem
When using setNativeProps to set the style of an object, the explicitly passed style (i.e. the style prop of a View), gets locked to its initial value. If it's changed, it will no longer update after 1-2 render passes. I believe this is because the props get internally "cached", but then it will always clobber the "new" style prop, thereby freezing it.

How to reproduce
Simplified test case: https://codesandbox.io/s/sweet-davinci-f1lut?file=/src/App.js

Steps to reproduce:

  1. Click the button, it should expand the square
  2. Notice that after a few clicks it stops expanding the square

Expected behavior
The box should continue expanding indefinitely

Environment (include versions). Did this work in previous versions?
Yes, this is a regression in #1939 / 0.15.2

  • React Native for Web (version): 0.15.2+

I am happy to submit a PR with tests in a few hours, but I believe reversing the order of style and previous style here will correct it https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/modules/usePlatformMethods/index.js#L27

@comp615
Copy link
Contributor Author

comp615 commented Apr 12, 2021

@piaskowyk curious why previous styles is the second arg in the array there instead of the first. I'm assuming there may be some other issue you were working around there.

@comp615 comp615 changed the title setNativeProps style is incompatible with also updating stype prop setNativeProps style is incompatible with also updating style prop Apr 12, 2021
@comp615 comp615 changed the title setNativeProps style is incompatible with also updating style prop setNativeProps with style is incompatible with also updating style prop Apr 12, 2021
@comp615 comp615 changed the title setNativeProps with style is incompatible with also updating style prop setNativeProps with style is incompatible with also updating style prop itself Apr 12, 2021
@necolas
Copy link
Owner

necolas commented Apr 12, 2021

Damn. I knew that PR was going to cause a regression, I had just lost context since stabilizing that method and there isn't test coverage of all the undefined RN behaviors implemented. So yeah adding tests sounds great

@necolas
Copy link
Owner

necolas commented Apr 12, 2021

I believe reversing the order of style and previous style here will correct it

That didn't work. But I think I have a fix...or at least I can produce the expected results for both issue test cases. No tests yet

@comp615
Copy link
Contributor Author

comp615 commented Apr 13, 2021

@necolas thanks for looking at this, I ended up in a bunch of meetings today so still haven't gotten further, but looks like you are on top of it. Happy to help test as I can once you have something. FWIW, on twitter.com this manifests as the Image cropper not being zoomable (maybe other manifestations but that's the only one I've found so far)

@necolas
Copy link
Owner

necolas commented Apr 13, 2021

I'm going to publish a 0.15.x patch that reverts the regression, and we can figure out how to accommodate any changes to the logic in a future minor release.

@necolas
Copy link
Owner

necolas commented Apr 13, 2021

Reverted in 0.15.7

rnike pushed a commit to VeryBuy/react-native-web that referenced this issue Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants