wix-incubator / remx

Opinionated mobx
https://wix-incubator.github.io/remx/
MIT License
224 stars 19 forks source link

Not Idiomatic MobX #32

Closed benjamingr closed 6 years ago

benjamingr commented 6 years ago

Hey,

idiomatic MobX

I don't think this is idiomatic MobX, at all. Idiomatic mobx is:

But @mweststrate or other members of MobX core might have a vastly different idea :)

Cons: doesn't scale very well due to not being a design pattern, can be implemented in lots of different ways, complex API due to large amount of features.

I don't think architecture is a one size fits all - your architecture or state management should not be a "design pattern". Architecture is different and should depend on what problem you're actually solving and what your code aims to solve.

I also don't think it's fair to say "scales very well" is a pro of Redux, I've had a terrible time scaling redux and consulting to companies who use/used redux. In fact, I think the main benefactor from large redux apps is consulting companies due to how hard it is to scale centralized state.

Moreover:

redux itself is really just an implementation of flux. Which itself is a close relative of MVC.

I don't think that statement is fair. Redux is not really an architecture (and neither is MobX), both solve the change detection problem for app-wide state. MobX with getters/setters and Redux by only changing the state in one (global'ish) place. MVC is an architecture that dictates how the interaction between said state should be structured which is orthogonal. As a side note no one is actually doing de-jure MVC anyway (which is why I liked how Angular called their thing MVW back then).

In your example:

We can't really use redux here because it will require us to rewrite everything, and will make our business logic and tests tightly coupled to redux itself.

Sure you can, you can do the classic "lift the setState to the reducer" - this is very common with Redux.

We can however use mobx. If we figure out a way to wrap all of our business logic with mobx's observables, actions, and our component as observer, we will achieve our requiremnents.

You only need to wrap things that are observed with @observable and things observing them with @observer. You don't need to do anything with actions if you don't want strict mode.

One major thing mobx does is impact all objects that it touches, in order to observe on their state.

True, but it is transparent, the fact it touches objects should be mostly invisible to everyone involved. The calling side and external code is unaware of the fact MobX is involved which is sort of the beautiful part.

DanielZlotin commented 6 years ago

I agree that the readme could use some work to better reflect our stance and the history of why we wrote this package, and you are welcome to PR changes, but the gist of it is this:

I (and others) couldn't do proper TDD with redux. Tried really hard, but no matter what I did, my tests knew about redux. That is where I tried to go about it in a naive way, forgetting existing frameworks. Just drive all business logic (pure js) with tests, and then find a way to connect it to update the react components.

This gave birth to polymorphic-redux. Then we figured it's still too much of a hassle and doesn't solve the problem completely, just hides it.

So next came Mobx, given that it's a really good and well known library (I think we can agree on that), I still found it's API sometimes confusing and I just wanted to work with something more straightforward, that uses Mobx's mechanism of triggering re-renders when needed, without any other feature or responsibility. The test was: let's write a simple app's business logic (I think it was yet another reddit client) in a good ol' TDD way without any concerns about frameworks, then finally connect it to some component and everything should just work™.

This meant I needed Mobx to be a simple plugin, and I needed it's API to reflect it. in fact, I was willing to remove degrees of freedom from Mobx, just to reflect what it should really do: be a plugin to the application, connecting the business logic to the UI, with as little code as possible, triggering re-renders only when needed.

So thus remx was born, hiding the Mobx implementation under a very simple and strict API. That's what I meant by calling it a Design Pattern. Clearly not a "gang of four" pattern, just a simple abstraction making Mobx an implementation concern. (Maybe I should have written "Opinionated Mobx"?) Mobx obviously is much more flexible and powerful, which is a good thing, just not what I was looking for.

I was looking for initialState, setters, getters and connect.

I should mention that here at Wix Mobile Guild, there are lots of different small teams working on different products, across the globe, all living under 1 big mobile app (written in react-native). They all have freedom in what they choose to use. What I try to do, is create some order in this chaos.

Turns out, this was a very good decision, because one of the first issues when working with remx was that Mobx's observables are leaking into places that don't expect it, causing real-world production problems. For example it broke lodash array operations. It broke when different teams (as explained above) turned out sharing the same observable object in 2 different versions of Mobx stores etc.

The fact the remx is just an abstraction gave us the freedom to deal with those problems easily(ish. its still software..), for example by using es6 Proxies as a layer between mobx-react and the state.

I still think it's a better approach and it allows you to focus on doing proper TDD with zero regard for the underlying frameworks. It allows you to defer decisions about which frameworks to use.

If you write (drive) your app with tests and then you find that you have no use for remx (or any other framework) I see it as a win.

benjamingr commented 6 years ago

I (and others) couldn't do proper TDD with redux. Tried really hard, but no matter what I did, my tests knew about redux.

I'm not sure why/how that's a problem, unlike MobX - your objects looking like "regular JS files" was never a thing for Redux anyway. You just write your tests as:

  1. Set state before
  2. Dispatch an action
  3. Observe state after action

This gave birth to polymorphic-redux. Then we figured it's still too much of a hassle and doesn't solve the problem completely, just hides it.

Yes - that's a little silly. A redux store is its own rolled OOP object - you dispatch actions (call methods) with payloads (parameters) - it's classic OOP message passing which is silly since the language already provides that (it has OOP built in) - redux just does this as the simplest thing to update state (all done in same place).

MobX does something a lot more reasonable (and clever) by actually detecting changes.

So next came Mobx, given that it's a really good and well known library (I think we can agree on that), I still found it's API sometimes confusing and I just wanted to work with something more straightforward, that uses Mobx's mechanism of triggering re-renders when needed, without any other feature or responsibility.

I'm not sure how the API is simpler than @observable - changes, @computed - derived change method, @observer - reacts to changes. All the rest is convenience for library authors.

I'd love to hear what issues you've found with the API so we can improve it though.

This meant I needed Mobx to be a simple plugin, and I needed it's API to reflect it.

I'm not sure why that is, my MobX unit tests are just regular unit tests - and are mostly unaware of MobX.

was that Mobx's observables are leaking into places that don't expect it, causing real-world production problems. For example it broke lodash array operations.

Can you elaborate on that?

I still think it's a better approach and it allows you to focus on doing proper TDD with zero regard for the underlying frameworks.

MobX isn't a framework - it's just a change detection library in disguise :D

Niryo commented 6 years ago

I'm not sure how the API is simpler than @observable - changes, @computed - derived change method, @observer - reacts to changes. All the rest is convenience for library authors.

I think that Remx solves all the pitfalls that are presented in the What does MobX react to section. Mobx is great, but it could be pretty tricky sometimes. Remx will pick up any changes to the state, even changes in nested props, added keys etc`.

amir-arad commented 6 years ago

I agree that the term "Idiomatic Mobx" is misleading. Mobx documentation clearly describes the appropriate ways in which to use its API. This is the Idiomatic meaning of the term "Idiomatic Mobx".

Perhaps the correct description for this library would be "Redux-Idiomatic Mobx" or "JS-Ideomatic Mobx".

@DanielZlotin - TDD does not mean you need to abstract away every single technology in your stack. The same way tests are coupled with the view layer API (React / DOM / native etc.) and ES6 API, they can be coupled with any other technology. You made a personal choice of working with Mobx as a simple™ plugin, which reflected your personal expectations and understanding of Mobx, Redux and TDD. That's fine and good, but it is far, far from being "Idiomatic Mobx" In my opinion.

That being said, Abstraction points add complexity to the overall system and are often less performant, but allows reasoning about (=implementing) each part separately. the separate reasoning often have hidden costs in itself (see John Carmack's excellent talk on the subject). This is why this trade-off is the heart of productive engineering, and there is no one-size-fits-all recipe (@benjamingr would say 'architecture'), even if you're doing TDD by the book. Many parts of the program Don't need unit testing. I think that the flow of the TDD approach streamlines these choices to the modular-but-complex side.

But in places where an abstraction point is desirable, it is better to look outside for an API that suits your needs and has already proven its ability to hide implementation than to write one yourself, right? Have you checked out atmover? perhaps offering an atmover-redux integration module would prove more TDD-Idiomatic than remx?

@Niryo - regarding the"solves all the pitfalls (...)" claim, let's take the first as an example: how exactly does Remx solves the "changing a non-observable reference" problem? is there any special feedback when a setter changes a non-observable reference? if there is, wouldn't it make more sense simply to trigger a state change event instead of providing said special feedback?

Niryo commented 6 years ago

@amir-arad Remx solves this idiomatically. You don't have any non-observable references, the setters are changing only the state object, and those changes will be picked up. Mobx doesn't tell you how to structure your app (and this is a good thing!), one can create a store with mixed observable values and non observable values and the pitfall becomes relevant.

amir-arad commented 6 years ago

I don't understand these claims.

import * as remx from 'remx';

const initialState = {
  loading: true,
  posts: {},
  selectedPosts: [],
};

const state = remx.state(initialState);
const setters = remx.setters({
 setLoading(isLoading) {
   state.exploading = isLoading; // <------ notice how exploading is not part of the original state
 },
 });
const getters = remx.getters({

 isLoading() {
   return state.loading; 
 },

});
export const store = {
  ...setters, ...getters
};
amir-arad commented 6 years ago

perhaps, when you use the term "idiomatic" you mean "opinionated"?

benjamingr commented 6 years ago

Let me phrase this, I am a MobX team member, @amir-arad is de-facto mobx core and Michel actually wrote MobX - that's 3 people here involved with the project (including Michel) saying that this isn't "idiomatic".

I don't think that this actually solves anything or that such a one-size-fits-all solution exists. You're welcome to think differently and that's great. I don't want to discourage you from building things - especially on top of MobX (rock on!).

My only problem is with the term 'idiomatic' and the claims made in the readme. I've found MobX to be simple, flexible and very usable without third party tools on top of it. I don't think wrapping it actually solves any of the "hard" problems me and @amir-arad are talking about while introducing an abstraction which adds cost to projects using MobX.

As for pitfalls with MobX - I've found those to be mostly people using MobX "as magic" rather than understanding how it works - once I figured out how it works I haven't really run into said pitfalls. The fact some things are observable and some aren't is a big feature IMO (for example, some things I want to be able to change outside of actions, but I want to reflect the fact they can't impact UI)

DanielZlotin commented 6 years ago

perhaps, when you use the term "idiomatic" you mean "opinionated"?

Yes, you're right. Opinionated.

We don't agree about a few fundamental axioms. When we're talking about tests, I (and @Niryo ) are talking about Unit tests, those that test business logic, without any notion of UI (react/dom/whatever). I also don't agree that you should/have/better to use a third party abstraction instead of your own. The whole point of abstractions are that they are tailor made to suit your use-case, as they represent your core business-logic concepts (Application-specific business rules), and they shouldn't introduce source-code dependencies on implementations. Those implementations can be third party dependencies, as long as they don't "pollute" the project with their intricacies.

Regarding your example, state.exploading will cause re-renders on components that use it.

I already gave other examples of missing features/bugs in Mobx. Lodash array operations are broken when working with observable arrays. You'll have to call toJS to solve this. Another example is when you have nested observable objects in 2 different stores. Just look at the tests in this project and you'll see all those edge cases.

@benjamingr As I already said, if I have to learn Mobx and it's intricacies before I write tests for my business logic, then I regard it as an obstacle to my TDD cycle (just as I regard redux as an obstacle), an obstacle I push to the side (outside the responsibilities of my business logic) by abstracting it away.

I would be happy to never have to use remx, and just use Mobx as a plugin, but as I said (and as the tests in this project have shown) it's not a 100% fiddle-free solution. Hence, remx.

amir-arad commented 6 years ago

In my opinion, this is a solid description of the meaning of "Ideomatic" in terms of code being language-idiomatic :

Idiomatic programming means that your program contains statements that are unique to the language; i.e., you actually use the expressive power (...) of the language in your programs.

( Source, feel free to google "Idiomatic code")

So "use the expressive power of the language in your programs" is exactly the opposite of abstraction. it means tightly coupling your code with the language's unique features.

The term "Idiomatic Mobx" natively translates to "working according to the Mobx Idiom, exploiting its unique features". However, The unique features of Mobx is exactly what remx is abstracting away from the user. I guess this is the source of confusion.

amir-arad commented 6 years ago

also

The whole point of abstractions are that they are tailor made to suit your use-case

NO. THE OPPOSITE.

amir-arad commented 6 years ago

how about you replace the word "Idiomatic" with the word "Opinionated" in this project's metadata (github, npm, README) and we'll call it a day?

Niryo commented 6 years ago

how about you replace the word "Idiomatic" with the word "Opinionated" in this project's metadata (github, npm, README) and we'll call it a day?

We will:) I guess it would have been better if we started this dialog by saying that we all agree about the incorrectness of the term "idiomatic mobx" :) You raised some other points that we disagree on, but not this one haha

benjamingr commented 6 years ago

As I already said, if I have to learn Mobx and it's intricacies before I write tests for my business logic, then I regard it as an obstacle to my TDD cycle

I maintain that you should probably learn how the libraries you use works (in general) before writing code with said libraries (and tests in your case). Note that the way MobX works with getters/setters is explained in one page (which you've linked to) and that contains all the caveats. After reading it and using MobX for a few hours it should be obvious what should/shouldn't work.

Your tests should definitely be aware of what code you're using. I don't think tests are hard with Redux or MobX - or that tests should be unaware of what your code is using.

I've found many issues with using Redux (outlined here: https://www.youtube.com/watch?v=VT6_v2lHi0Q and here https://www.youtube.com/watch?v=QT8KRoXBlZE ) but TDD was never one of them. Tests were always just "get initial state, dispatch action, observe updated state" - sometimes with multiple actions or sagas.

If you're trying to "hide aware MobX" that's fine - just be aware of not hiding too much :)

I also want to emphasize I agree with Amir's statements about what abstraction is above.

Niryo commented 6 years ago

Guys, before we continue to argue, let me just say that we are soon going to solve the issue in the title ("Not Idiomatic MobX "). We are going to clean the docs and we will extract the agenda stuff into a separate blog post, where we could continue to argue in the comments section if you want:)

benjamingr commented 6 years ago

Thanks. Feel free to close the issue once you've updated the description and README.