jina-ai / dashboard

Interactive UI for analyzing Jina logs, designing Flows and viewing Hub images
https://dashboard.jina.ai
Apache License 2.0
118 stars 61 forks source link

Implement ImmutableJS for Redux store #133

Closed BastinJafari closed 3 years ago

BastinJafari commented 3 years ago

Copy from https://redux.js.org/recipes/using-immutablejs-with-redux#why-should-i-use-an-immutable-focused-library-such-as-immutablejs

Immutable.JS was designed to provide immutability in a performant manner in an effort to overcome the limitations of immutability with JavaScript. Its principle advantages include:

Guaranteed immutability# Data encapsulated in an Immutable.JS object is never mutated. A new copy is always returned. This contrasts with JavaScript, in which some operations do not mutate your data (e.g. some Array methods, including map, filter, concat, forEach, etc.), but some do (Array’s pop, push, splice, etc.).

Rich API# Immutable.JS provides a rich set of immutable objects to encapsulate your data (e.g. Maps, Lists, Sets, Records, etc.), and an extensive set of methods to manipulate it, including methods to sort, filter, and group the data, reverse it, flatten it, and create subsets.

Performance# Immutable.JS does a lot of work behind the scenes to optimize performance. This is the key to its power, as using immutable data structures can involve a lot of expensive copying. In particular, immutably manipulating large, complex data sets, such as a nested Redux state tree, can generate many intermediate copies of objects, which consume memory and slow down performance as the browser’s garbage collector fights to clean things up.

Immutable.JS avoids this by cleverly sharing data structures under the surface, minimizing the need to copy data. It also enables complex chains of operations to be carried out without creating unnecessary (and costly) cloned intermediate data that will quickly be thrown away.

You never see this, of course - the data you give to an Immutable.JS object is never mutated. Rather, it’s the intermediate data generated within Immutable.JS from a chained sequence of method calls that is free to be mutated. You therefore get all the benefits of immutable data structures with none (or very little) of the potential performance hits.

DavidSanwald commented 3 years ago

Okay, it seems like there are certain things you passed over:

  1. What are the costs? You mentioned the benefits but there's always a downside.
  2. What are concrete problems in the codebase, that will benefit from introducing immutable? What immediate and/or long term impact outweighs the costs?
  3. Considering the previous questions are there any alternatives, to immutableJS?

Since I don't know enough about the dashboard or its current state, I will leave 2. and 3., which depends on 2., to somebody else. But to 1.:

  1. Whenever your code has to deal with those immutable data structures, you can't use normal es6 syntax anymore. You need to awkwardly deal with the ImmutableJS API. That means that more and more code in your codebase will be coupled to API details of ImmutableJS. If you attempt to avoid this, you end up having to call toJS() everywhere during every render. Normal es6 syntax does not work anymore, every developer needs to know individual methods of ImmutableJS, just to get a simple property from an object.
  2. While most developers know immutable update patterns and how to use Redux, fewer developers will know how to benefit from immutable data structures, while avoiding potential traps/problems.

There might be good enough reason to consider immutableJS. But I think posting all the benefits and leaving out the potential drawbacks just below the pros you copied in the very same article is a problematic approach.

BastinJafari commented 3 years ago

I had the thought to use immutable for Redux, because I was chasing a bug for half of the day that had something to do with accidentally pointing one property of the new state to a property of the old state. It would relieve me of a lot of work, if I could just make a copy of the old state and set the properties on this new object without having to think about it at all. Had in mind that the library should only be used inside of the redux state handling. When data get in or out, it should be a regular JS object again. So it wouldn't be the case that more and more code will get coupled to the library.

Do you have bad experiences with calling toJS() with every render? What would be your solution to prevent bugs like the one I described above?

DavidSanwald commented 3 years ago

First of all: Sorry, I realized my previous post was way too negative and confrontational. My bad. I think it's important to be always open to technologies that can improve the project, even if there are initial costs and overall risks.

I think you have good intentions wanting to restrict the use of ImmutableJS to the redux state. But how would you do that? Every piece of state has to pass through map state to props every render/reconciliation. There's the idea of using HOCs, so you only call toJS if the props actually change. That means you need to be very selective, where you actually connect a component (and therefore need to wrap it). Naively calling toJS() in mapstatetoprops is catastrophic. Using the HOC technique could work or could not much help at all. Depends on the ratio of total props to changed props per render.

I don't know the bug you chased, so I can't really give any input on this. But since the repo is public, maybe you can tell me which bug and share the commit, where you fixed it. Maybe I can provide something useful then.

In general: I think the most problems come from not using data in an immutable way (which could still just be using mutable data and not mutating it by convention) in the Flux stores made things very complicated. Otherwise, you would not need to dispatch all those events all the time. In terms of immutable alternatives: ImmerJS is now used in the redux toolkit, very popular and for sure worth considering. Using proxies comes with its own costs but not even considering it would be a mistake. I think it's the standard solution for immutability, that's currently recommended by the redux team: https://redux.js.org/redux-toolkit/overview That does not mean it's necessarily the right choice or automatically a better choice than ImmutableJS.

General things:

jina-bot commented 3 years ago

This issue is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 4 days

BastinJafari commented 3 years ago

Hey David, thank you very much for pointing out immerJS. We decided to go with it!

Roshanjossey commented 3 years ago

Closing this as it has been resolved in the pull requests linked above.