miklschmidt / capacitor.js

MIT License
8 stars 2 forks source link

remove get() and getIn() from the store's interface object #2

Open miklschmidt opened 9 years ago

miklschmidt commented 9 years ago

When switching to immutable-js, we found several places in our code where a store might dereference an object by using another content store, this is pretty hard to do and ends up being ugly (too magical) if done in get() and getIn(). This way the user is forced to implement property accessors which can then properly handle the data. Takes a little extra time but pays off when refactoring. Breaks backwards compatibility.

miklschmidt commented 9 years ago

I can't come to terms with this change. If we just handle dereferencing in the stores properly, this shouldn't be much of an issue. What i'm working on atm is having Store.getIn() call Store.get() internally so the dereferencing is done automatically (another feature i'm working on). Unless you want to change Store.getSomething() to return Store.get('somethingelse') while refactoring, which you really shouldn't because that would be confusing, i can't see the benefit. All i can think of is scenarios in which you shouldn't be in the first place. @kastermester help me out here.

kastermester commented 9 years ago

I would still go with the change of removing them from the interface. It is much better encapsulation to require an accessor function. However, it is entirely possible to mitigate some of the tedious coding that one has to create in order to make these accessor functions, and I would suggest that we add this to the store (ie. an array of things that are ok to get and getIn using a get* function. This way, when/if the logic around the specified property needs to change - one can simply implement the method in a regular fashion.