Skip to content

HostText needs to copy over from current if it is unchanged #17538

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

Merged
merged 1 commit into from
Jan 3, 2020

Conversation

sebmarkbage
Copy link
Collaborator

stateNode is not one of the fields that gets copied over by createWorkInProgress.

This is bad for persistent mode since it's not the same instance.

In HostComponent we happened to always end up transferring the current stateNode when we bail.
However, we didn't do this for HostText.

Found by @JoshuaGross in Fabric.

TODO: Unit tests.

…tent mode

stateNode is not one of the fields that gets copied over by createWorkInProgress.

This is bad for persistent mode since it's not the same instance.

In HostComponent we happened to always end up transferring the current stateNode when we bail.
However, we didn't do this for HostText.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 6, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 21747e9:

Sandbox Source
flamboyant-cloud-2lhd0 Configuration

@sizebot
Copy link

sizebot commented Dec 6, 2019

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 21747e9

@JoshuaGross
Copy link
Contributor

Thanks for fixing this so quickly!

@sizebot
Copy link

sizebot commented Dec 6, 2019

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 21747e9

@JoshuaGross
Copy link
Contributor

@sebmarkbage Is there a plan to add tests to this and land / is anyone driving it? This will become/is a blocker for Fabric TextInput.

@gaearon
Copy link
Collaborator

gaearon commented Dec 18, 2019

@JoshuaGross I was planning to do a sync after the holidays. This is currently blocked by lack of tests but we can land by then if @sebmarkbage doesn't write any.

@JoshuaGross
Copy link
Contributor

@gaearon Looks like we should just merge this in, since there haven't been any updates / tests

@gaearon gaearon merged commit 59f21f1 into facebook:master Jan 3, 2020
@gaearon
Copy link
Collaborator

gaearon commented Jan 3, 2020

I got it in. I will run a sync once I’m back from PTO — on Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants