fix: check if its first render before updating from props #419
Conversation
|
cc @j-piasecki |
hannojg
force-pushed
the
fix/race-condition-props-state-updates
branch
from
1d33c03 to
0fc17f6
Compare
ios/MarkdownCommitHook.mm
Outdated
| auto plainString = | ||
| std::string([[nsAttributedString string] UTF8String]); | ||
| if (plainString != textInputProps.text) { | ||
| if (textInputState.getRevision() == State::initialRevisionValue && plainString != textInputProps.text) { |
There was a problem hiding this comment.
I think we can even skip comparing state text with props text, but it's been a while since I've touched this code so I'm not sure. We can merge this as-is to fix the issue and investigate it further later.
There was a problem hiding this comment.
I think we can even skip comparing state text with props text,
Yeah I was wondering the same. I think the only case where state == props on first render is when the input is empty 🤔 (but not 100% sure either)
|
Unfortunately I get a crash here :( Just run the example app with new arch enabled on iOS and press "Reset" or "Clear" button There's another crash on main but I'm working on it edit: okay i think it's my fault
|
|
let me test on the new arch 🤔 thanks for checking! // Edit: oh okay, so you say the bug isn't caused by this PR? @tomekzaw |
|
Cool, do you think when we merge this one soon and could do a new version? asking because I am wondering whether I should create a pr with a patch in newdiot, or If I should just wait a few minutes and then we can do a PR with a version bump? |
|
@hannojg Yeah I think we can merge your PR soon, just wanted to test it out. No need for patch right now. Right now I'm waiting for the CIs on my PR. |
|
@hannojg Could you please merge main? |
…n into fix/race-condition-props-state-updates
|
Done! |
Details
See: Expensify/App#43983 (comment)
It's possible that the props aren't updated yet on the JS side, while the native state already is. In that case we'd drop the native state for a stale prop value which is incorrect. So we should make sure to only run this code on the first render.
Related Issues
Expensify/App#43983
Expensify/App#44185
Manual Tests
I think its hard to add a specific test for this change.
I verified that the condition is executed on the first render.
Linked PRs
I don't understand this point 😅