Skip to content
This repository was archived by the owner on Dec 19, 2020. It is now read-only.

Add redux, redux-thunk and react-redux #123

Closed
p2kmgcl opened this issue Feb 19, 2020 · 8 comments
Closed

Add redux, redux-thunk and react-redux #123

p2kmgcl opened this issue Feb 19, 2020 · 8 comments
Labels

Comments

@p2kmgcl
Copy link

p2kmgcl commented Feb 19, 2020

[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 performing fetch 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.

@p2kmgcl p2kmgcl added the rfc label Feb 19, 2020
@wincent
Copy link
Contributor

wincent commented Feb 19, 2020

I'd be extremely reluctant to add redux in any kind of official of endorsed way (ie. in a shared frontend-js-redux-web module, or as part of frontend-js-react-web) because I think that in well over 95% of the JS code I've seen in liferay-portal, the complexity simply isn't called for. Adding it would tacitly encourage its use and we'd wind up with code that — in the majority of cases — is far more complicated and boilerplatish than it needs to be, and would present a needless and steep learning curve to any engineer who comes into contact with it for the first time.

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:

Redux is used by too many react developers without thinking twice! Applied mindlessly like that, redux does more harm than good!

In response to your point:

We have the exact same API, so migrating everything would be just changing the import declarations.

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.

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.

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.

@julien
Copy link
Contributor

julien commented Feb 19, 2020

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 yarn upgrade and we also have to think about backwards compatibility. This implies a lot of work from developers as well as QA engineers .

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 🦄 ...

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.

That's not exactly how I see it: I'd say we started by using React.createContext, useContext, useReducer and custom hooks that allow using thunks (which was enough for a start), and at some point, "we" effectively copied redux's API without really thinking about the real problem which was using context effectively - we could have asked help from @wincent and others to solve this problem before doing this, but that's just my two cents.

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.

We have the exact same API, so migrating everything would be just changing the import declarations.

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.

@wincent
Copy link
Contributor

wincent commented Feb 19, 2020

for example this

There are a few little gems in an article that is linked to from that one, @julien — "Application State Management with React":

There are hundreds of "easier redux" abstractions on npm ... This is the reason that I only ever used redux on one project ... Here's the real kicker, if you're building an application with React, you already have a state management library installed in your application.

(Both articles by our good friend, Kent C Dodds.)

@p2kmgcl
Copy link
Author

p2kmgcl commented Feb 19, 2020

@wincent I've been reading the articles you linked, and this is what I saw on them:

  • Redux requires lots of boilerplate to have something working
  • React context allows you sharing state from different parts of an application
  • GraphQL improves the backend-frontend communication
  • We do not have 'reactn' or any other global state library
  • Observables may be another good solution, but we still need a custom implementation for state management
  • This amount of boilerplate that we have generated ends in lots of ordered code, which I think it's a good thing if we are supposed to maintain this application for years
  • All react context examples show how you can happily create a counter and reuse it, which is not as complex as the state we have
  • Sharing all the information we need with contexts will force us to create almost the same amount of code
  • We cannot use GraphQL

Redux is used by too many react developers without thinking twice! Applied mindlessly like that, redux does more harm than good!

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.

Once Redux is in liferay-portal, we can never remove it

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.

"And that issue that you link to is highly speculative"
"then there won't be any "maintenance""

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.

You may be wondering why I am pushing so hard against Redux unlike some other examples like react-dropzone.

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.

@p2kmgcl
Copy link
Author

p2kmgcl commented Feb 19, 2020

hey @julien

Secondly, I'm surprised that this comes up as an issue on this repository, since we've talked about this offline numerous times.

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).

saying that we use redux 🦄

I don't want to say "we are using redux", I've never wanted. I just find it useful.

That's not exactly how I see it: I'd say we started by using React.createContext, useContext, useReducer and custom hooks that allow using thunks (which was enough for a start), and at some point, "we" effectively copied redux's API without really thinking about the real problem which was using context effectively

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).

We have the exact same API, so migrating everything would be just changing the import declarations.

That's doesn't seem very realistic to me.

I'm pretty sure about this. I can try to prepare a small demo.

If people are able to build huge React apps without redux, I think we should also be able to do so.

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

@bryceosterhaus
Copy link
Member

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 useContext and useReducer couldn't solve. So ultimately, I think until I see a glaring problem that only redux can solve, I wouldn't advocate in using it.

also the boilerplate in redux drove me crazy. So much code needed for simple actions.

@wincent
Copy link
Contributor

wincent commented Feb 20, 2020

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 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).

@wincent
Copy link
Contributor

wincent commented Mar 25, 2020

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.

@wincent wincent closed this as completed Mar 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants