Possible improvements?

[AriaFallah]

@mweststrate not really sure if this is an abuse of mobx since I'm using the private methods. I was wondering if you could take a look.

I'm using the reportChanged functionality to make it work

db.object[key].$mobx.atom.reportChanged()

[mweststrate]

Using internals is bad sign indeed :)

It is not entirely clear to me whey changes are reported, I would have expected reportObserved in the chain function? Or is this because users are allowed to modify data returned from a chain? Otherwise I think you can base your chain on just db.object[key].slice(), which gives a defensive copy and makes sure your autoruns are re-run when the collection is changed again in the future.

[AriaFallah]

@mweststrate Well the original implementation of the library is here: https://github.com/typicode/lowdb

And the idea is that you can both read and write to the DB by chaining lodash methods

// Read Example
db('posts')
  .chain()
  .filter({published: true})
  .sortBy('views')
  .take(5)
  .value()

// Write Example
db('posts')
  .chain()
  .find({ title: 'low!' })
  .assign({ title: 'hi!'})
  .value()

My problem with the original library was that I couldn't call the lodash methods on an Observable Array so I wanted a way to be able to modify the underlying collection with lodash methods while also keeping the array emitting the observable events.

Does db.object[key].slice() allow for this?

---

Edit: These two tests fail when using just slice as I expected. The lodash methods seem unable to modify the data inside the observable collection. So even the first assign([1, 2, 3]), which I want to just set the initial value of the internal collection, doesn't work.

test('It works when calling a single method', (t) => {
  let i = 0
  db('b').assign([1, 2, 3])
  autorun(() => i += noop(db('b').value()[0]))
  db('b').assign([5, 2, 3])
  t.is(i, 2)
})

test('It works when chaining', (t) => {
  let i = 0
  db('c').assign([1, 2, 3])
  autorun(() => i += noop(db('c').value()[0]))
  db('c').chain().find((x) => x === 1).assign(4).value()
  t.is(i, 2)
})

---

To answer your questions:

I would have expected reportObserved in the chain function

It's called implicitly when either you call .value at the end of a chain, or just use a single method like db('posts').assign([1, 2, 3]). I also make sure not to call reportObserved for methods that don't mutate your data such as db('posts').find((x) => x === 1) by checking if state of the underlying array has changed. I noticed MobX already seemed to do this because, before I implemented that feature, the .find call wasn't triggering autorun, but I wanted to be safe about it.

[are] users allowed to modify data returned from a chain?

Well yes after it's returned they can do whatever they want with it, but I don't think I would want that to emit observable events. I only want observable events emitted for the methods that are a part of the chain and mutate the underlying collection.

[mweststrate]

Ok thanks for the info, the use case is now clear to me. ObservableArrays are sadly no real arrays until ES6 proxies will become wide available, as it is not possible in ES5 to customize real arrays. Real arrays simply cannot be observed. Slicing turns a observable array back into a real array, but as you will have noticed, it won't "write through", meaning that this approach is only viable for read operations.

I have to think about this a bit more, but these directions might be interesting:

1. provide the whole chain api as smaller wrapper functions around the backing observable array. If there are many functions allowed this could be quite some work although all the read api's could be wrapped generically.
2. Trick lodash into thinking that ObservableArrays are real arrays. That should work fine if it uses normal array methods, but won't work if it uses methods from Array.prototype.
3. Let the chain start with a sliced copy, and copy it back to the original array at the end of the chain. But I doubt whether you can reliably determine that moment.

Probably not very satisfying, but I hope that answers the question.

[AriaFallah]

I think I already have approach 1 working. Would you mind reviewing the source, and letting me know if my implementation is a good one?

Basically if the lodash chain mutates the underlying array it'll call reportChanged(), and if it doesn't it'll call reportObserved(). I think this is similar to how you do it yourself after reading through the code for ObservableArray.

I just commented all of it so it's easier to understand as well.

[mweststrate]

Hmm, since you have the checksum anyway, you could make it maybe even cleaner by slicing, after the chain check whether it has changed, if if that is the case use .replace(changedCollection) on the observable array. The slice will make sure that reportObserved() is called (or maybe that should not happen always?), and replace will automatically invoke reportChanged itself.

I think btw that there is some consistency check to see whether the internal values are not changed from the outside, I would have to double check that. (and verify whether those are really needed)

[AriaFallah]

@mweststrate I think this is a good idea, but I have one concern.

Wouldn't using slice() + replace() be much less performant than calling the private methods?

1. I'd be using slice() when I already have direct access to the underlaying data, and I might be accidentally triggering reportObserved when I don't mean to.
2. Then if I make any changes I use that sliced array with replace() to mutate the underlying data. I would do double the work with mutations. Once to actually change the data, and another time to put it into the observable array.

The advantage is that I don't need to call the the reportObserved and reportChanged methods, which makes it much easier to adhere to the standards of your library.

---

My new question would be what's the downside to explicitly using the $mobx private methods for observation?

From reading the code, I think the one downside is that it won't call

adm.changeEvent.emit

which I can actually fix if necessary.

And the other is that you're doing some weird things with the array length that I'm not really sure about the point of. For example I've seen how you're reserving a buffer of 1000 initially, and I'm not sure yet, but this might break that in some cases.

---

So really I think the conclusion I've reached is to explicitly call reportObserved() because sometimes I want to report an observation without creating a new copy of the array, but to use replace() instead of reportChanged() so I can adhere to whatever you're doing behind the scenes.

Does that make sense?

[mweststrate]

@AriaFallah the trouble you might run into from changing internal semantics might be much bigger then the possible performance overhead. In these kind of situations (accessing private internals from other libs) it holds more than ever: premature optimization is the root of all evil. Privates are for a reason private; they might change between any version. They might be renamed, or even be minified in minified builds. I simply cannot guarantee future compatibility of private methods.

Besides that, it won't suprise me that slice + replace is actually faster in most common cases. A full array slice is a really cheap operation because browsers can very efficiently clone a complete collection at once. Besides that, after slicing, each read is slightly cheaper as reading from observable arrays is actually a little more expensive (each property access will run reportObserved) and with slicing that happens only once.

The same holds for replace, replacing a full observable array has almost the same overhead as just pushing a single item to it. Both run reportChanged and emit a single event. So replacing only once might again be cheaper than applying multiple small mutations in the chain.

[AriaFallah]

@mweststrate I came to the same conclusion after I edited my question about 50 times 😅 .

I think the only problem now is the figuring out implementation of using replace().

Thank you for all your help.

[AriaFallah]

@mweststrate Seems to be working now with the public API 😄 . Thanks.

[mweststrate]

Cool!

Op wo 20 apr. 2016 om 00:50 schreef Aria Fallah notifications@github.com:

@mweststrate https://github.com/mweststrate Seems to be working now with the public API 😄 . Thanks.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/AriaFallah/mobx-store/issues/1#issuecomment-212159045