AriaFallah / mobx-store

A data store with declarative querying, observable state, and easy undo/redo.
MIT License
282 stars 9 forks source link

Reducing the size of the package by limiting the available lodash methods to a subset. #8

Closed hnordt closed 8 years ago

hnordt commented 8 years ago

You had an awesome idea. Thank you for opensourcing it!

I would like to suggest to work if a subset of lodash methods, requiring the entire lodash library may me cumbersome for some projects.

AriaFallah commented 8 years ago

@hnordt

Yeah I wanted to get it working before I started looking at perf and how to fix the problem of large package size.

I'm looking into a solution that's not too limiting, but the biggest problem is just the fact that I don't think it should be my decision what lodash methods are exposed.

This looks somewhat promising, and I'll be looking into it more very soon:

https://medium.com/making-internets/why-using-chain-is-a-mistake-9bc1f80d51ba#.eqjvwsovb

AriaFallah commented 8 years ago

@hnordt @mweststrate what do you think of this?

The current API depends on chaining, which requires the entire lodash library, and is

store('x').chain().method().method2().value()

instead we change the API so the user has to do

import { method, method2 } from 'lodash/fp' 
store('x', method(), method2())

and they use https://github.com/lodash/babel-plugin-lodash

That way they only use the methods they decide to include.

hnordt commented 8 years ago

I liked the idea of lodash fp. 👍

hnordt commented 8 years ago

@AriaFallah do you think it's ok that small to medium apps use one store instead multiple?

After you published this library I realized I can have only one store, that's actually the whole state:

class State {
  constructor() {
    this.map = map()
    this.get = this.map.get.bind(this.map)
    this.set = this.map.set.bind(this.map)
  }
  merge = (key, value) => {
    return this.set(key, {
      ...this.get(key),
      ...value
    })
  }
}

const state = new State()

state.set('color', 'red')
state.set('person', { hnordt: { age: 26 } })
state.merge('person', { AriaFallah: { age: 'unknown'  } })
AriaFallah commented 8 years ago

@hnordt

I honestly can't think of many scenarios when you would need multiple instances of mobx-store

You can just use different keys for your different data sources like

const store = mobxstore()

store('people').doStuff()
store('colors').doStuff()
store('other').doStuff()

Also I would recommend not using mobx-store yet as the API is about to change if I can fix this issue with importing all of lodash.

hnordt commented 8 years ago

Nice, so mobx store is my app state machine :)

It's like Redux, but with mobx store instead reducing/transforming state manually, it's automatically handled by MobX.

Redux

Create Store => Create Action => Dispatch Action => Reduce/Transform State => Read State

MobX/MobX Store

Create Store => Create Action => Dispatch Action => Read State

AriaFallah commented 8 years ago

@hnordt haha yeah that was the idea behind this. I'm sure there's more stuff to add to this store to make it a lot better that I'm currently not thinking of as well.

hnordt commented 8 years ago

@AriaFallah

What about supporting middleware like Redux does? Something like:

class State {
  constructor(middleware = f => f) {
    this.map = map()
    this.middleware = middleware
  }
  get = key => {
    return this.middleware('get', { key }, this.map.get(key))
  }
  set = (key, value) => {
    return this.middleware('set', { key, value }, this.map.set(key, value))
  }
  merge = (key, value) => {
    return this.set(key, {
      ...this.get(key),
      ...value
    })
  }
}

const middleware = (action, params, nextValue) => {
  if (action === 'get' && typeof nextValue === 'string') {
    return `Next value: ${nextValue}`
  }
  return nextValue
}

const state = new State(middleware)
AriaFallah commented 8 years ago

Opened #10 for continuing the discussion for middleware there.

AriaFallah commented 8 years ago

11 Fixes this