Migrating hiccup to pure react component usage (First step) #18574
Conversation
| @@ -0,0 +1,29 @@ | |||
| (ns react-native.pure | |||
|
|
||
| (def settings-category (quo.theme/with-theme category-internal)) | ||
| (defn settings-category | ||
| [{:keys [label data container-style blur?]}] |
There was a problem hiding this comment.
example of pure functional component with hooks
Jenkins BuildsClick to see older builds (46)
|
flexsurfer
force-pushed
the
feature/pure-rn-experiment-2
branch
2 times, most recently
from
dd562c5
to
e561216
Compare
flexsurfer
changed the title
flexsurfer
requested review from
J-Son89,
briansztamfater,
OmarBasem,
ilmotta,
alwx,
cammellos,
vkjr,
ajayesivan,
clauxx,
ibrkhalil,
BalogunofAfrica and
ulisesmac
This comment was marked as resolved.
This comment was marked as resolved.
|
💯 @flexsurfer! Looks really cool. I like the direction this is going in, specifically the moving away from reagent and using ratoms for local state. There's a few places I noticed where ratoms were passed down to (sometimes very nested) children and updated from there, making it hard to track what caused a state change. I wonder how difficult it might be to migrate such cases. Also, are there any performance metrics that would show the benefits of the pure approach versus hiccup+reagent? I wonder how much performant it is and what are our current perfomance bottlenecks. |
|
Thank you for working on this PR @flexsurfer! I would like to review this PR and experiment with the abstractions you are proposing before judging. But before reviewing, I think this costly decision to migrate away from Reagent should be accompanied by a set of steps for other contributors to reproduce the performance improvements you have measured. The theory you are sharing makes sense, but we need numbers and a way to reproduce them ourselves :) I think it's not just any % improvement that can justify the changes being proposed. Are we talking 10%, 20% lower times to render on average? Which % would justify abandoning Reagent? There are other cards on the table IMO before we settle on a solution that can't be easily undone:
|
|
Very interesting work @flexsurfer ! I like that the API is similar to what we currently have. I have a lot of thoughts about dropping Reagent. Reagent is a very important library for the Clojure community, and we aren't the only ones having problems with it, so I want to share some ideas:
Related to the implementation in this PR: I have to test it, but just by reading, I believe the Reagent's performance cost is mainly due to:
We are still suffering by It'd be very helpful to check how similar is the performance of this implementation compared to Reagent. So as everyone said, some performance measurements to evaluate if this is the right path are needed. |
|
thanks @pavloburykh , fixed and yes it will definitely will take some time to review this PR ;) |
For me, the significant changes I am referring is the sheer volume of work to make the proposed changes, not necessarily the type of changes. |
29% of end-end tests have passedFailed tests (33)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (14)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
To reiterate @flexsurfer, just as I said in a private Google doc: Reagent has an overhead and we all know, but this doesn't mean I agree we should remove it, not without proof of perf gain to justify the effort and the architectural change.
My comment is about this PR alone, not about past work, but thanks for sharing the links. Since you're proposing we rewrite a lot of code now and in follow-up PRs and abandon the Reagent way of life, I do believe you should back up your claims so we can all assess if it's worth or not to move forward with your proposal.
How significant? Could you share the steps you took to verify this and to reach this conclusion?
To undo in the future, we would need to rewrite all files again to use Reagent, bring back all Reagent abstractions and update all guidelines, as well as align with all collaborators the effort to undo the changes. And probably fix a few bugs in the way due to the reverse migration. It's definitely costly to undo this tech stack decision in the future. That's one big reason why we are being more conservative while reviewing this PR.
Hum, I think it's a stretch to say one is the result of the other. So far, it has been a joy to use Reagent in status-mobile, except a few places. Very seamless experience coming from Reagent on the web. The convoluted code you mention with reactive atoms is more the fault of poor code than that of Reagent itself, and also technical skills from developers. I believe we'll continue to have the same code complexity, on average.
I didn't know about this decision to move to React states over Reagent atoms? Is this written down somewhere?
Why do you think that? I'm not proposing using UIx, I'm proposing being open for other solutions and comparing them. UIx can bring other benefits for DX, such as compile-time linters when using
Macros are a unique advantage of Lisps. Clojure libs are exploring this realm mainly due to the ability to run compile-time transformations and compile-time checks/linting, something that can't be easily done with hiccup, or with just functions. We shouldn't dismiss macros as if they're generally bad. Other Lisps explore them a lot more.
I understand how you feel, but abandoning Reagent is more than just changing vectors to lists. Generally, if the changes are controversial, I think it's best if we first agree on the approach and only then jump to the code.
This point is confusing to me. What do you mean by "doesn't need Reagent"? Quo components use Reagent for local state and for all the other niceties Reagent provides. It allows us to have a coherent code base, where the same practices are applied in components and in screens. This is important so that the developer only has to learn one way of doing things. Also, we like to refer to Quo as a library, but it's tightly coupled with the screens, so it's not really a component library to me. Hence, making decisions using this loose concept confuses me.
I disagree with this strategy. Do we have this goal as a team/org or is this your personal view? The product has yet to prove itself in the real world, so obsessing with UI performance will just slow us down and take away devs from doing something more important (including rewriting screens to be simpler and faster). The chat screen is primarily slow because of the way it's written, my gosh it's super convoluted and full of dynamic elements changing size and triggering re-renders. Devs should team up with designers to come up with a reasonable middle-ground in terms of functional & non-functional requirements. I worked in many React apps that were slow because of how badly they were written and/or how complex the UI was. It's the same thing for status-mobile. Also, we should seriously consider the fact that some screens just won't perform well with CLJS + RN, nor with just RN. Pushing for RN and CLJS everywhere in the app can lead to workarounds trying to fit a square in a circle.
Performance is bad while developing for Android under GNU/Linux. And on release builds on Android it has never been great since I touched the app on Aug 2022. I think Android's low performance is well established within the team. |
73% of end-end tests have passedFailed tests (11)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (35)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
thanks @ilmotta, @J-Son89 , the longer we wait, the more work will need to be done, changing brackets isn't that big of a job, it's more like changing formatting, i was trying to keep the approach and API as similar to the current as possible. Actually, it's not only about performance, and might be it even not the first reason, if we look at the reasons why big projects move away from reagent it is because it makes code more familiar to js (RN) devs. React library is evolving (for example concurrent renderer) and it is better to use the latest versions and techniques, and the best way to do it is to use it directly without wrappers And regarding performance, setting screen changes in this PR shows visible improvements, it's visible for the user that the screen opens faster. and also measured in js profile, ~200ms vs ~300ms , but it's still hard to measure final numbers because we still have part of the app in hiccup with reagent atoms. From these points in the complex, it is obvious to me why we should move away from the reagent now and not wait until the code base becomes bigger and more complex. And it's not only about performance, but it's also about bad maintenance of library and devs experience |
73% of end-end tests have passedFailed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Passed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
| @@ -42,16 +42,15 @@ | |||
| (defn use-theme | |||
| "A hook that returns the current theme context." | |||
| [] | |||
| (utils.transforms/js->clj (rn/use-context theme-context))) | |||
| (keyword (rn/use-context theme-context))) | |||
There was a problem hiding this comment.
btw we had initially made this return a map so that the api is easier to extend if we ever need to.
use-theme-value would get the result directly and you could just use that here.
100% of end-end tests have passedPassed tests (1)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
|
92% of end-end tests have passedFailed tests (2)Click to expandClass TestDeepLinksOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (44)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
@flexsurfer I'll put some thought tomorrow and share. Thank you. The actual reason that makes me consider abandoning Reagent is the number of bugs our team produces due to incorrect usage of Reagent and our sub-optimal usage of it (perf wise), not really because it has a small(ish) overhead. My experience with React since the early days tells me it won't be much different with raw React, we'll kind of just switch the types of bugs... We just need to look at the parts of the app heavy on animation, the complexity isn't really due to Reagent, it's because of the animation code that infests everything. We take Reagent away from these places, the code will have the same degree of complexity on average (Composer for example, but many others).
This PR feels kind of stuck because of the lack of evidence to support performance claims to justify abandoning Reagent. When you suggest eliminating a basic library many people invested time learning and it's widely used, it's also natural to be questioned more. IMO, before this PR was opened, consensus in the team should have been reached without actual prod code. We don't need prod code to agree about removing Reagent and we don't need prod code to come up with a solution/API. Proposals are cheaper and we can do many. I also see you refactored a lot of code in this PR, which makes our job to review harder. It also creates this tension where, because you already invested too much time, you want to see them into
I see. The main reason seems to be that Reagent feels outdated, requires convolutions to use hooks, confuses JS devs (most of the team), and so on. This is problematic, I agree. That's a bigger problem than Reagent's performance which could be in fact be improved considerably. We don't take advantage of its most basic performance feature that's based on diffing props. For instance, we pass anonymous functions down the entire component tree. No wonder Reagent has to do a ton of extra work.
I appreciate the honesty about the difficulty to measure. I profiled the app using this PR as well with Hermes. I couldn't see a conclusive difference to justify eliminating Reagent, maybe because we have other parts using Reagent as you said. But some numbers could have been provided if the PR focused on refactoring a simpler screen. Even a single, heavy component in a preview screen could give interesting & reproducible results.
I'm curious. Would you mind sharing examples of companies or projects that moved away from Reagent? btw, I think we need more developers profiling the app and understanding the trade-offs. That might seem like a waste of time, but I think it will create a stronger team and it will pay off. |
|
@ilmotta @J-Son89 thank you very much for your very valuable feedback, now i understand that I acted a little recklessly by transferring this experimental PR into a merge-ready one, presented it as something just ready for review and then ready to be merged, in my head i see the big picture, i have 100% confidence it will work and we should move there, but probably as you correctly said, there should a plan and understanding of how and why we are going there, and this PR cannot be merged in this form. so at first, I was focusing on performance when I started looking into slow laggy settings screens, step by step I found out that the problem was in the number of elements on the screen, I knew that reagent adds some overhead to each component, so I decided to try to replace it, and at that moment I already had thoughts and knowledge that's it's better to use hooks, and actually reagent is not needed, so I moved fast, and decided to check this, and then on the halfway numbers showed 60% improvement I decided that's enough, and I can stop because I did already too many changes. And for that reason i decided to push it just as performance improvement. But now I think we should go the other way, I didn't notice that I had done the work of replacing reactive atoms with hooks because I considered it self-evident, and replacing So now I think first we need to agree, that we use a lot of functional components, and mixed hooks and reagent atoms are not great, and for most js devs reactive atoms and reagent forms variants are not easy to understand and properly use. So it would be better to replace reagent atoms with hooks 
this is work to be done ^ and then, i think we could keep hiccup, and just use our custom compiler for reagent, where we will use only functional components without reactions, wdyt? PS: I'm currently writing an article with technical details that I will publish later |
Hey, this is rather interesting stuff @flexsurfer. Seems like a nice middleground if we can keep Reagent, but make it play nice with hooks and perform better. We should embrace hooks because they're not going anywhere. Do you believe you can eliminate or reduce the overhead of functional components with Reagent? Have you looked at this issue? Does it give you ideas? Provide a React hook for using ratoms/reactions as part of public API reagent-project/reagent#545 Or are you just inclined to eliminate reactions and period?
When you mention "custom compiler" it confuses me a little bit, because Reagent does offer an API for a custom compiler since mid 2021. I have frequently wondered where this could lead us or if it could help us, but never played with it in status-mobile. I imagine the compiler could be optimized for our mobile constraints. Should probably be on our deck of cards too.
Correct, we do have many functional components and we'll eventually use more and more.
Maybe others should jump in this conversation and answer this question, maybe it's actually fine for most. Based on my experience so far in this project, my impression is that most devs use Reagent atoms correctly after an initial learning curve of mistakes. The form-1, form-2 variant is a very simple concept based on closures, but mistakes are made every now and then. I see most also learn to do this effectively after a learning curve. But I can't speak for others on this matter. There are many mistakes made with Reagent that seem to be tied to devs's knowledge of CLJS, so we should be careful with any cause & effect analysis when we blame Reagent.
Super nice! Would love to read it Andrey. |
Hey @flexsurfer, for sure I can definitely got on board if we take a more gradual approach and there's other developers working on some of this too. Who knows where our blind spots are on this! |
| @@ -29,7 +29,7 @@ | |||
| {:height (:size opts) | |||
| :width (:size opts) | |||
| :borderRadius (style/get-border-radius (:size opts)) | |||
| :backgroundColor (colors/resolve-color (:customization-color opts) :dark)}) | |||
There was a problem hiding this comment.
Hi @flexsurfer, why these values reversed here?
There was a problem hiding this comment.
idk , probably there was a bug before?
| [react-native.core :as rn] | ||
| [react-native.pure :as rn.pure])) | ||
|
|
||
| (defn view-pure |
There was a problem hiding this comment.
In other places are just replacing with pure version, why we are keeping both here? Is it WIP?
There was a problem hiding this comment.
yes, overwise i have to migrate all children for all overlay usage
| [^js data] | ||
| (quo/category | ||
| {:list-type :settings | ||
| :container-style {:padding-bottom 0} | ||
| :blur? true | ||
| :data (.-item data)})) |
There was a problem hiding this comment.
- Is
^jsis necessary here? - Why data is changed here?
There was a problem hiding this comment.
here not, in 18th line is enough, what do you mean by data changed? you mean why it's a js object now? it's a good question, somehow i missed these changes in the description of the PR, it should be a separate change, optimisation of flat-list
There was a problem hiding this comment.
what do you mean by data changed?
In first case, we are passing data directly but in second we are using .-Item
There was a problem hiding this comment.
we use flat list directly no wrappers anymore
Thus, by transitioning to the exclusive use of React components and states, we will eliminate unnecessary computations of Hiccup markup by Reagent from the JavaScript engine, thereby freeing it up. This approach will also allow us to work solely with React states, removing any confusions.
In this PR new package was introduced
in this implementation, we keep Clojure data structures for props and styles, so the only overhead is converting cljs data to js, but this also was optimized by pre-initializing keys cache
good thing is that new components can be used in the hiccup, so we can migrate gradually
example of component migration
current
[rn/view {:style (style/container circle-size customization-color neutral? theme)} [text/text {:accessibility-label :wallet-user-avatar :size (:font-size (size properties)) :weight (if monospace? :monospace (:font-weight (size properties)))new
so basically the only change is replacing
[with(, but to remove confusion for now better to use also different namespace for rn componentsrn.pureso the next thing is re-frame subscriptions, new functions was introduced in the re-frame utils namespace
basically, it just updates state when subscriptions is changed
And reactive atoms should be replaced by react states
Example of migration
current implementation
new
also, notice the theme context usage, now we're using it directly as react context in the components
theme (quo.theme/use-theme), there is a known limitation for now, because our parent components still hiccup we have to create functional component as element for now similar how :f> is used. for example, for settings-views it will look likealso, one more confusion might be, that it's not possible to use hiccup inside new components, we might have these situations for now while we have a mixed approach, it won't be an issue later. For now most likely place to get this problem is the usage of text component when children are passed as parameters and its a hiccup
So in this PR i wanted to see how it works, and how difficult it will be to migrate our code, and was pretty smooth and simple, and i went pretty deep, but decided to stop, because it's already too many changes for one PR, so I'll continue work in another PR, but will start from migrating root components
One more thing is react native libraries, they also should be migrated, for now i just added pure versions for components in their namespaces for example blur
(def view-pure (rn.pure/get-create-element-fn (.-BlurView blur)))reanimated
(def view-pure (rn.pure/get-create-element-fn (.-View reanimated))), btw we should look into reanimated separately , probably we could improve it because we don't have hiccup anymore etcalso one thing, svg was migrated and it should work faster now also because no sense to have it in hiccup
Summary: In fact, we are already trying to use react state instead of reagent atoms for local states, so when we discuss not using reagent, the only change for us is to use list instead of vector, is this really such a big deal? honestly i did a lot of work in this PR, and i didn't notice any difference in writing code, highlight is different because its a list now, but probably it's even better because it highlights a component name now