Enable functional components by default #18820
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (28)
|
flexsurfer
changed the title
flexsurfer
requested review from
Parveshdhull,
ilmotta,
siddarthkay and
ulisesmac
ulisesmac
reviewed
Feb 14, 2024
| [rn/touchable-without-feedback {:on-press (when backdrop-dismiss? close-bottom-sheet)} | ||
| [rn/pressable {:on-press (when backdrop-dismiss? close-bottom-sheet)} |
There was a problem hiding this comment.
Please double check this.
I noticed some styles were breaking when I tried to remove all the touchable-without-feedback wrapping a reanimated/view
There was a problem hiding this comment.
yes thanks, touchable-without-feedback doesn't add a container, so if the child uses absolute positioning or flex it will work differently with pressable it's better not to use touchable-without-feedback at all
There was a problem hiding this comment.
I agree I had to deal with weird crashes in rn-73 upgrade due to touchable-without-feedback -> #18563 (comment)
Its better if we use rn/pressable everywhere
flexsurfer
force-pushed
the
feature/enable-functional-components-by-default
branch
from
c13834b
to
7090507
Compare
flexsurfer
force-pushed
the
feature/enable-functional-components-by-default
branch
from
d79681a
to
8540b93
Compare
Parveshdhull
approved these changes
Feb 15, 2024
siddarthkay
approved these changes
Feb 15, 2024
19% of end-end tests have passedFailed tests (38)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (9)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
19% of end-end tests have passedFailed tests (38)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (9)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
|
ilmotta
reviewed
Feb 16, 2024
There was a problem hiding this comment.
@flexsurfer: We had a lengthy discussion in the past about not enabling functional components by default because they will make everything slower. I remember I was in favor of enabling by default even with the performance hit because of the improved ergonomics to use hooks. Don't throw tomatoes at me ;)
But enabling functional components by default in this PR will make the app slower overall, which is something we definitely don't want. Could you elaborate on the impact of this PR? I understand many components are functional and more will come, but there will always be a good portion of the UI code that won't ever need hooks.
My guess is that this PR is also part of a plan to allow all local Reagent atoms to be replaced by use-state in a future PR, but is this something we reached agreement on?
|
hey @ilmotta yes, you were right, back then i didn't have a big picture, now i understand we need to move to functional components, i did a research and now i have a full picture, you can find technical details in this article https://www.notion.so/Reagent-under-the-hood-8de546ea8c96468baf64118938a62a23 Also I've created an EPIC with steps here #18800 And this PR is the first step, from the reagent code and from this PR i didn't noticed huge difference in the performance, probably there is some overhead, but it's not visible. Later when we take full control, functional components should be even faster.
we have an agreement in the crew i guess, at least no objections, so we were waiting for you, you can leave your concerns or objections here, and i will organize the meeting later this week so we could agree and put this in our guidelines |
flexsurfer
force-pushed
the
feature/enable-functional-components-by-default
branch
from
b436739
to
2227c56
Compare
Indeed, that part stays since we're using re-frame. |
J-Son89
approved these changes
Feb 20, 2024
flexsurfer
force-pushed
the
feature/enable-functional-components-by-default
branch
from
2227c56
to
241d186
Compare
|
thank you @VolodLytvynenko :) |
Hi @flexsurfer You're welcome. It would be appreciated if you could share any known areas that might be impacted by this PR. |
|
@VolodLytvynenko entire UI is impacted, so just quick visual inspection on how UI looks and works |
58% of end-end tests have passedFailed tests (19)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (28)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
85% of end-end tests have passedFailed tests (6)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
33% of end-end tests have passedFailed tests (4)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
vkjr
approved these changes
Feb 23, 2024
25% of end-end tests have passedFailed tests (3)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Passed tests (1)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
hi @flexsurfer, |
100% of end-end tests have passedPassed tests (3)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
|
|
@flexsurfer thank you for PR. Sorry for the testing delay. I have been waiting for e2e results. PR is ready to be merged |
flexsurfer
deleted the
feature/enable-functional-components-by-default
branch
ibrkhalil
pushed a commit
that referenced
this pull request
Feb 25, 2024
* enable functional components by default * e2e: updated counter component locator --------- Co-authored-by: Yevheniia Berdnyk <ie.berdnyk@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #18802