-
Notifications
You must be signed in to change notification settings - Fork 24
Add redux, redux-thunk and react-redux #123
Comments
I'd be extremely reluctant to add As the following plethora of articles shows, engineers tend not to be very good at distinguishing when they actually need something; it is all too easy to fall into the "when all you have is a hammer, everything looks like a nail" problem:
I think with Redux is likely to suffer particularly badly from the "hammer-nail" problem. As one of those articles says:
In response to your point:
I want to reference some general points on the cost of adding any dependency, not just Redux: Once Redux is in liferay-portal, we can never remove it. We are forever wedded to its code size, its security footprint, its legal ramifications, its decisions to make or not make semver breaking changes — and not just Redux, but whatever dependencies it may rely on now and in the future. Precisely because it is so popular, we can expect that people will build large quantities of code on top of that API, magnifying the costs of any breaking changes made to its APIs (react-redux is currently at v7). Because Liferay maintains long-lived 7.x branches, we can expect the cost to be multiplied when/if any module that we add gets backported to other branches.
I don't buy this argument. The page editor is working and do we have any issues in the existing implementation that would be solved by moving to Redux? And would the move be cheaper than just fixing the issue? I am inclined to think that once the basic state management flow works (and it seems that it does), then there won't be any "maintenance". Stuff will just work. It does now. And that issue that you link to is highly speculative, and if implemented, will rely on Proxies, which are not supported in IE and never will be (cannot be polyfilled), so we wouldn't benefit from it anyway. You could make the argument for bring Redux into layout-content-page-editor alone in the sense that would mitigate the downsides, but sadly, it still wouldn't entirely eliminate them. If we open the door to projects independently bringing their own versions of Redux with them, we wind up having to fight to keep them in sync, and it would add pressure on us to consolidate the versions into a central location, forcing us to effectively "bless" the usage of Redux in liferay-portal which I am just not convinced would be net-beneficial for Liferay. You may be wondering why I am pushing so hard against Redux unlike some other examples like react-dropzone. It's because I think the potential downside of making a mistake about react-dropzone is much more contained; but if we screw up with Redux, it could be something that we have to live with painfully, and extensively, for years. The Facebook Ads teams built some of the largest SPA React apps in the world — literally thousands of React components in each — and did it without Redux. It's just not clear that we need it. |
First of all, I agree with what @wincent wrote about this. I also want to point out that managing dependencies in liferay-portal is not a trivial task, upgrading them is sometimes not as easy as running Secondly, I'm surprised that this comes up as an issue on this repository, since we've talked about this offline numerous times. Having worked on the page editor project I don't really understand what benefits this would bring apart from saying that we use redux 🦄 ...
That's not exactly how I see it: I'd say we started by using I'm sure that there are other ways to separate the state or use Context more efficiently that we could have come up with, for example this, and I'm sure there are a lot of other developers who have had to solve the same problem.
That's doesn't seem very realistic to me. If people are able to build huge React apps without redux, I think we should also be able to do so. |
There are a few little gems in an article that is linked to from that one, @julien — "Application State Management with React":
(Both articles by our good friend, Kent C Dodds.) |
@wincent I've been reading the articles you linked, and this is what I saw on them:
I don't recommend using redux because it's being used by a lot of developers: I have thought about this lots of times, not only once or twice, I tried starting from scratch from a small application and forced myself not using redux until making new changes was terrible. So please do not say or think that this is me trying to copy random boilerplates without considering other solutions. For me the real benefit of using this library is not only the problem that it solves, but the community and tools that lives behind it. I think this consideration is similar that the reasons that made us move to react.
I know, and believe me when I say that it also worries me. I know that I am not the responsible of handling this problems, but I also would feel terrible if this results in being a bad idea.
The idea of linking that issue was not showing some fix that we need, just presenting some difficult problem that a community is trying to solve. I am not saying that we are not able to solve these problems by ourselves, but I don't know if we will have time to do so.
I wasn't, I understand that these to libraries are completely different and that redux is a much more "generic" library that can propagate without control. Maybe it can be restricted, maybe it can't. But I know we are not speaking about the same topic. |
hey @julien
Because I still think that using redux is a good idea and I wanted to both request it formally and know if someone shares my opinion (out of the Madrid office).
I don't want to say "we are using redux", I've never wanted. I just find it useful.
It's true that we could have come up with some different solution with contexts. But I think that starting using useReducer and useThunk led us to replicate a redux-like application structure. Maybe it was me, I don't know, but that is what we have know. I don't think we just copied redux API without thinking about the real problem, I think we had a big global state and we needed to get stuff from it (selectors) and send information to the backend (thunks).
I'm pretty sure about this. I can try to prepare a small demo.
I know, but I don't think that building huge apps without redux is something that people should be proud of. There are huge projects, some with redux, some without it |
I'll try to be concise with my thoughts and wold be happen to expand on them more if necessary. I used redux heavily in Loop and Faro and it was integral to those applications. It was helpful at the time, but since the hooks API was released in react, I haven't seen a circumstance yet where redux solved a problem that also the boilerplate in redux drove me crazy. So much code needed for simple actions. |
I can speak to that because I'm the one who did it 😂 I very intentionally adopted a Redux-like application structure because I wanted to respect the pattern that you'd followed in the original version of the page editor (where you also very carefully implemented a Redux-like pattern, Pablux!). I really saw no point in trying to invent an entirely new paradigm just because we were starting a rewrite of the app from scratch: rather, I wanted to minimize the "distance" between the original and the rewrite by preserving the basic shape as much as possible, while ensuring that we stayed in harmony with "the React way" (which meant, obviously, using hooks). If we were starting a brand new application with no history, I wouldn't have gone for the extreme level of structure and boilerplate that we we have in the page editor. There would be no action creators, no thunks, no action types spread over multiple files, no combined reducers, no plug-in architecture etc; I'd just have started with the simplest tools that React provides (state, then context/reducers etc, if needed) and gradually introduced additional complexity and structure only when it became evident that it was necessary. But none of that applied in the case of the page editor: we had a working app already, and your team was both accustomed to that level of structure/boilerplate, and felt that it was useful. We mirrored the Redux API — again, very intentionally, just like you did with Pablux — to reduce the potential effort of a future possible migration. But as far as I can tell, we still haven't seen a good argument for what value Redux would bring us, and whether that value would justify the costs (neither in the context of the page editor, nor in the context of liferay-portal as a whole). The argument you're putting forward here @p2kmgcl sounds a lot like, "we did this development, and in the end what we have ended up looking like Redux — surprise!? — , so we therefore we should switch to Redux, as it will be easy". It shouldn't be surprising that it looks like Redux, though, should it? We designed it that way at every step (in both versions of the editor). |
Closing because the conversation here has petered out. We can re-open this for another round of discussion in the future if necessary, but for now I want to de-clutter the issues list by removing some things that aren't going to lead to immediate action. |
[x] Before making a proposal, have you used the issue search functionality?
https://bundlephobia.com/result?p=redux@4.0.5
https://bundlephobia.com/result?p=redux-thunk@2.3.0
https://bundlephobia.com/result?p=react-redux@7.1.3
What is your proposal?
Adding redux, redux-thunk and react-redux to Liferay Portal so we can have a shared state in large components like the Page Editor.
Although react contexts are a good solution when you need to share some information between components, it can lead to performance issues when there this state becomes too complex or there are many components depending on it.
redux
solves the "big state issue" with a simple state-machine pattern, organizing state changes with actions and relying on inmutability when updating it. These patterns are being used in lots of component-based solutions (vue, xstate, mobx, even react, etc.) as they fit really well with the component pattern.react-redux
solves the "state sharing issue" by injecting parts of the state only to the components that need it. Initially it worked the HOC pattern, embracing the recommended reuse techniques, and now it provide some hooks that allow getting parts of the state and sending new changes.redux-thunk
is a tiny solution for managing asynchronous code when updating the state (the most common usecase is performingfetch
to update or get information from the backend). It introduces "thunks" as functions that can dispatch actions after doing some work.Why would adopting this proposal be beneficial?
As the page editor grew, we found the need of sharing a big state between different metal components (this was way before the react migration). We tried to create some custom patterns to share state between components, but maintaining while developing more features was nearly impossible: the state was growing too big and we had many components sharing this information with unwanted re-renders.
Now that we have migrated everything to react we have adopted similar patterns (see store, reducers, actions, thunks directories) that replicate
redux
's ecosystem to solve the same problem. We have the exact same API, so migrating everything would be just changing the import declarations.So, if we have successfully created a copy of redux using react, why would we need to use redux instead? There are two main reasons: as I said before, maintenance, and problem solving. State injection is not trivial, and the redux community is already trying to solve some problems that have also happened to us. Our current implementation is being maintained by ourselves, a product team, and we shouldn't invest too much time in solving problems that have already been solve by the community.
What are the alternatives to this proposal?
I wanted to add a formal request to include redux, but there are some other state management libraries mentioned in #44. However keep in mind that redux is the defacto solution for many react projects, and applying it to existing code would be much easier.
The text was updated successfully, but these errors were encountered: