[Fabric] use state in paragraph component #24873

ericlewis · 2019-05-15T23:15:13Z

Summary

Updates the paragraph component to use State instead of Local Data, part of the path to a Fabric TextInput 💯

Changelog

[General] [Changed] - Fabric: Use State instead of Local Data for Paragraph

Test Plan

Run RNTester with Fabric enabled, view that text still renders.

ReactCommon/fabric/components/text/paragraph/ParagraphState.cpp

shergin

It's almost perfect, Eric! ❤️

Could you please a couple of nits I mentioned and I would love to land in on Monday.

React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm

ReactCommon/fabric/components/text/paragraph/ParagraphShadowNode.cpp

ReactCommon/fabric/components/text/paragraph/ParagraphState.h

shergin · 2019-05-16T18:15:42Z

React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm

@@ -8,7 +8,7 @@
 #import "RCTParagraphComponentView.h"

 #import <react/components/text/ParagraphComponentDescriptor.h>
-#import <react/components/text/ParagraphLocalData.h>
+#import <react/components/text/ParagraphState.h>


Those are not alphabetically sorted now.

If you can run clang format on that with config from the repo, that would be great. Otherwise I can do it for you.

@shergin ran yarn clang-format (complains about a lot of files!) but weirdly, nothing to say about the ordering. manually fixed it.

shergin

Thank you!! ❤️

I will land this on Monday!

ReactCommon/fabric/components/text/paragraph/ParagraphShadowNode.cpp

facebook-github-bot

@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-05-20T16:55:06Z

This pull request was successfully merged by @ericlewis in 6ec19ab.

^{When will my fix make it into a release? | Upcoming Releases}

shergin · 2019-05-20T21:35:01Z

@ericlewis Unfortunately, the change broke Fabric Android. Could you please figure out why?

Make sure you are using the changes from the actual commit (which is unlanding right now, btw), not from this PR because it was landed with some changes.

ericlewis · 2019-05-25T10:31:52Z

Hadn’t forgotten about this. I have a feeling I may know what happened. Sent via Superhuman iOS ( https://sprh.mn/?vip=ericlewis777@gmail.com )

…

On Mon, May 20 2019 at 5:35 PM, < ***@***.*** > wrote: @ericlewis ( https://github.com/ericlewis ) Unfortunately, the change broke Fabric Android. Could you please figure out why? Make sure you are using the changes from the actual commit (which is unlanding right now, btw), not from this PR because it was landed with some changes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( #24873?email_source=notifications&email_token=AAFEVRZUX62URSOZ7JGPG2TPWMKRHA5CNFSM4HNHV56KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2EL2Y#issuecomment-494159339 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAFEVRZR5EZK4IBGI2ESNILPWMKRHANCNFSM4HNHV56A ).

Summary: All props to Eric Lewis! cc ericlewis This revert of revert of land of changes originally published in #24873 (with some slight fixes). The change removes usage of LocalData from the `<Text>` component. After this change we only have ---one (maybe two)--- three components left using LocalData. Reviewed By: mdvacca Differential Revision: D15962376 fbshipit-source-id: 19f41109ce9d71ce30d618a45eb2b547a11f29a2

Summary: Updates the paragraph component to use State instead of Local Data, part of the path to a Fabric TextInput 💯 ## Changelog [General] [Changed] - Fabric: Use State instead of Local Data for Paragraph Pull Request resolved: facebook#24873 Differential Revision: D15410979 Pulled By: shergin fbshipit-source-id: 3c9517d2495a64c4dbd213b6efb5ff55287900e3

Summary: This sync includes the following changes: - **[4ea064eb0](facebook/react@4ea064eb0 )**: Don't fire passive effects during initial mount of a hidden Offscreen tree ([#24967](facebook/react#24967)) //<Andrew Clark>// - **[2c7dea736](facebook/react@2c7dea736 )**: Implement Offscreen in Fizz ([#24988](facebook/react#24988)) //<Andrew Clark>// - **[49f8254d6](facebook/react@49f8254d6 )**: Bug fix for <App /> vs. <Counter /> ([#24972](facebook/react#24972)) //<davidrenne>// - **[6b28bc9c5](facebook/react@6b28bc9c5 )**: test: Throw custom error instead of relying on runtime error ([#24946](facebook/react#24946)) //<Sebastian Silbermann>// - **[9bd0dd4c1](facebook/react@9bd0dd4c1 )**: test(react-debug-tools): Improve coverage of currentDispatcher.current setter ([#24945](facebook/react#24945)) //<Sebastian Silbermann>// - **[59bc52a16](facebook/react@59bc52a16 )**: Add 4.5.0 release to eslint rules CHANGELOG ([#24853](facebook/react#24853)) //<Sebastian Silbermann>// - **[cfb6cfa25](facebook/react@cfb6cfa25 )**: Reused components commit with timing as new ones //<Andrew Clark>// - **[679eea328](facebook/react@679eea328 )**: Extract layout effects to separate functions //<Andrew Clark>// - **[41287d447](facebook/react@41287d447 )**: Use recursion to traverse during "reappear layout" phase //<Andrew Clark>// - **[697702bf3](facebook/react@697702bf3 )**: Use recursion to traverse during "disappear layout" phase //<Andrew Clark>// - **[02206099a](facebook/react@02206099a )**: Use recursion to traverse during passive unmount phase ([#24918](facebook/react#24918)) //<Andrew Clark>// - **[f62949519](facebook/react@f62949519 )**: [Transition Tracing] Rename transitionCallbacks to unstable_transitionCallbacks ([#24920](facebook/react#24920)) //<Luna Ruan>// - **[7a4336c40](facebook/react@7a4336c40 )**: Use recursion to traverse during passive mount phase //<Andrew Clark>// - **[bb1357b38](facebook/react@bb1357b38 )**: Wrap try-catch directly around each user function //<Andrew Clark>// - **[de3c06984](facebook/react@de3c06984 )**: Move flag check into each switch case //<Andrew Clark>// - **[f5916d15b](facebook/react@f5916d15b )**: [Transition Tracing][Code Cleanup] Delete Marker Name Change Tests ([#24908](facebook/react#24908)) //<Luna Ruan>// - **[fa20b319f](facebook/react@fa20b319f )**: [Transition Tracing] Code Cleanup ([#24880](facebook/react#24880)) //<Luna Ruan>// - **[5e8c1961c](facebook/react@5e8c1961c )**: [Transition Tracing] onMarkerProgress ([#24861](facebook/react#24861)) //<Luna Ruan>// - **[b641d0209](facebook/react@b641d0209 )**: Use recursion to traverse during layout phase //<Andrew Clark>// - **[a1b1e391e](facebook/react@a1b1e391e )**: Wrap try-catch directly around each user function //<Andrew Clark>// - **[3df7e8f5d](facebook/react@3df7e8f5d )**: Move flag check into each switch case //<Andrew Clark>// - **[b8c96b136](facebook/react@b8c96b136 )**: Move ref commit effects inside switch statement //<Andrew Clark>// - **[e225fa43a](facebook/react@e225fa43a )**: [Transition Tracing] Don't call transition callbacks if no transition name specified ([#24887](facebook/react#24887)) //<Luna Ruan>// - **[dd2d65227](facebook/react@dd2d65227 )**: [Transition Tracing] Tracing Marker Name Change in Update Warning ([#24873](facebook/react#24873)) //<Luna Ruan>// - **[80208e769](facebook/react@80208e769 )**: [Transition Tracing] Add onTransitionProgress Callback ([#24833](facebook/react#24833)) //<Luna Ruan>// - **[30eb267ab](facebook/react@30eb267ab )**: Land forked reconciler changes ([#24878](facebook/react#24878)) //<Andrew Clark>// - **[5e4e2dae0](facebook/react@5e4e2dae0 )**: Defer setState callbacks until component is visible ([#24872](facebook/react#24872)) //<Andrew Clark>// - **[8e35b5060](facebook/react@8e35b5060 )**: [Transition Tracing] Refactor Code to Remove OffscreeInstance TODOs ([#24855](facebook/react#24855)) //<Luna Ruan>// - **[deab1263a](facebook/react@deab1263a )**: [Transition Tracing] Change Transition Type Passed Pending Transitions ([#24856](facebook/react#24856)) //<Luna Ruan>// - **[82e9e9909](facebook/react@82e9e9909 )**: Suspending inside a hidden tree should not cause fallbacks to appear ([#24699](facebook/react#24699)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions c1f5884...4ea064e jest_e2e[run_all_tests] Reviewed By: philIip, NickGerleman Differential Revision: D39305648 fbshipit-source-id: 627ead5035c77fbc902b306e17897e425ad7fb99

Summary: This sync includes the following changes: - **[4ea064eb0](facebook/react@4ea064eb0 )**: Don't fire passive effects during initial mount of a hidden Offscreen tree ([facebook#24967](facebook/react#24967)) //<Andrew Clark>// - **[2c7dea736](facebook/react@2c7dea736 )**: Implement Offscreen in Fizz ([facebook#24988](facebook/react#24988)) //<Andrew Clark>// - **[49f8254d6](facebook/react@49f8254d6 )**: Bug fix for <App /> vs. <Counter /> ([facebook#24972](facebook/react#24972)) //<davidrenne>// - **[6b28bc9c5](facebook/react@6b28bc9c5 )**: test: Throw custom error instead of relying on runtime error ([facebook#24946](facebook/react#24946)) //<Sebastian Silbermann>// - **[9bd0dd4c1](facebook/react@9bd0dd4c1 )**: test(react-debug-tools): Improve coverage of currentDispatcher.current setter ([facebook#24945](facebook/react#24945)) //<Sebastian Silbermann>// - **[59bc52a16](facebook/react@59bc52a16 )**: Add 4.5.0 release to eslint rules CHANGELOG ([facebook#24853](facebook/react#24853)) //<Sebastian Silbermann>// - **[cfb6cfa25](facebook/react@cfb6cfa25 )**: Reused components commit with timing as new ones //<Andrew Clark>// - **[679eea328](facebook/react@679eea328 )**: Extract layout effects to separate functions //<Andrew Clark>// - **[41287d447](facebook/react@41287d447 )**: Use recursion to traverse during "reappear layout" phase //<Andrew Clark>// - **[697702bf3](facebook/react@697702bf3 )**: Use recursion to traverse during "disappear layout" phase //<Andrew Clark>// - **[02206099a](facebook/react@02206099a )**: Use recursion to traverse during passive unmount phase ([facebook#24918](facebook/react#24918)) //<Andrew Clark>// - **[f62949519](facebook/react@f62949519 )**: [Transition Tracing] Rename transitionCallbacks to unstable_transitionCallbacks ([facebook#24920](facebook/react#24920)) //<Luna Ruan>// - **[7a4336c40](facebook/react@7a4336c40 )**: Use recursion to traverse during passive mount phase //<Andrew Clark>// - **[bb1357b38](facebook/react@bb1357b38 )**: Wrap try-catch directly around each user function //<Andrew Clark>// - **[de3c06984](facebook/react@de3c06984 )**: Move flag check into each switch case //<Andrew Clark>// - **[f5916d15b](facebook/react@f5916d15b )**: [Transition Tracing][Code Cleanup] Delete Marker Name Change Tests ([facebook#24908](facebook/react#24908)) //<Luna Ruan>// - **[fa20b319f](facebook/react@fa20b319f )**: [Transition Tracing] Code Cleanup ([facebook#24880](facebook/react#24880)) //<Luna Ruan>// - **[5e8c1961c](facebook/react@5e8c1961c )**: [Transition Tracing] onMarkerProgress ([facebook#24861](facebook/react#24861)) //<Luna Ruan>// - **[b641d0209](facebook/react@b641d0209 )**: Use recursion to traverse during layout phase //<Andrew Clark>// - **[a1b1e391e](facebook/react@a1b1e391e )**: Wrap try-catch directly around each user function //<Andrew Clark>// - **[3df7e8f5d](facebook/react@3df7e8f5d )**: Move flag check into each switch case //<Andrew Clark>// - **[b8c96b136](facebook/react@b8c96b136 )**: Move ref commit effects inside switch statement //<Andrew Clark>// - **[e225fa43a](facebook/react@e225fa43a )**: [Transition Tracing] Don't call transition callbacks if no transition name specified ([facebook#24887](facebook/react#24887)) //<Luna Ruan>// - **[dd2d65227](facebook/react@dd2d65227 )**: [Transition Tracing] Tracing Marker Name Change in Update Warning ([facebook#24873](facebook/react#24873)) //<Luna Ruan>// - **[80208e769](facebook/react@80208e769 )**: [Transition Tracing] Add onTransitionProgress Callback ([facebook#24833](facebook/react#24833)) //<Luna Ruan>// - **[30eb267ab](facebook/react@30eb267ab )**: Land forked reconciler changes ([facebook#24878](facebook/react#24878)) //<Andrew Clark>// - **[5e4e2dae0](facebook/react@5e4e2dae0 )**: Defer setState callbacks until component is visible ([facebook#24872](facebook/react#24872)) //<Andrew Clark>// - **[8e35b5060](facebook/react@8e35b5060 )**: [Transition Tracing] Refactor Code to Remove OffscreeInstance TODOs ([facebook#24855](facebook/react#24855)) //<Luna Ruan>// - **[deab1263a](facebook/react@deab1263a )**: [Transition Tracing] Change Transition Type Passed Pending Transitions ([facebook#24856](facebook/react#24856)) //<Luna Ruan>// - **[82e9e9909](facebook/react@82e9e9909 )**: Suspending inside a hidden tree should not cause fallbacks to appear ([facebook#24699](facebook/react#24699)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions c1f5884...4ea064e jest_e2e[run_all_tests] Reviewed By: philIip, NickGerleman Differential Revision: D39305648 fbshipit-source-id: 627ead5035c77fbc902b306e17897e425ad7fb99

[Fabric] use state in paragraph

626f9b5

ericlewis requested a review from shergin May 15, 2019 23:15

Fix comment

9e07fb4

ericlewis commented May 15, 2019

View reviewed changes

ReactCommon/fabric/components/text/paragraph/ParagraphState.cpp Outdated Show resolved Hide resolved

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels May 15, 2019

shergin suggested changes May 16, 2019

View reviewed changes

ericlewis added 2 commits May 16, 2019 14:10

Simplify.

1babc81

almost missed android

35a3ddb

shergin reviewed May 16, 2019

View reviewed changes

shergin approved these changes May 16, 2019

View reviewed changes

ReactCommon/fabric/components/text/paragraph/ParagraphShadowNode.cpp Outdated Show resolved Hide resolved

ericlewis added 2 commits May 16, 2019 15:34

Simplify

b74ba60

alphabetical

402a3e5

facebook-github-bot approved these changes May 20, 2019

View reviewed changes

facebook-github-bot closed this in 6ec19ab May 20, 2019

react-native-bot added the Merged This PR has been merged. label May 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fabric] use state in paragraph component #24873

[Fabric] use state in paragraph component #24873

ericlewis commented May 15, 2019 •

edited

shergin left a comment

shergin May 16, 2019

ericlewis May 16, 2019

shergin left a comment

facebook-github-bot left a comment

react-native-bot commented May 20, 2019

shergin commented May 20, 2019

ericlewis commented May 25, 2019 via email

[Fabric] use state in paragraph component #24873

[Fabric] use state in paragraph component #24873

Conversation

ericlewis commented May 15, 2019 • edited

Summary

Changelog

Test Plan

shergin left a comment

Choose a reason for hiding this comment

shergin May 16, 2019

Choose a reason for hiding this comment

ericlewis May 16, 2019

Choose a reason for hiding this comment

shergin left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented May 20, 2019

shergin commented May 20, 2019

ericlewis commented May 25, 2019 via email

ericlewis commented May 15, 2019 •

edited