Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[question] deep state, destructuring & action verbosity/DRY #333

Closed
leeoniya opened this issue Aug 18, 2017 · 49 comments
Closed

[question] deep state, destructuring & action verbosity/DRY #333

leeoniya opened this issue Aug 18, 2017 · 49 comments
Labels
discussion question How do I do that?

Comments

@leeoniya
Copy link

leeoniya commented Aug 18, 2017

hey guys,

i don't believe there is something like concise notation for deep destructuring:

const x = ({a.b.c}) => c.foo;

most examples with hyperapp use a flat state, so they look simple. but the app size will grow rather quickly and become less DRY as you have to repeat the state structure in multiple places. adding just one level to the counter example results in a lot more repetitive action code, or any code that can return partial state (but must be aware of the entire ancestor state structure):
https://codepen.io/anon/pen/ZJrrGW?editors=0010

attempts to make it smaller [understandably] don't quite work:
https://codepen.io/anon/pen/MvQQje?editors=0010

thoughts?
thanks!

@andyrj
Copy link
Contributor

andyrj commented Aug 18, 2017

@zaceno has a widget pattern that helps with deeply nested state I believe, he posted it to slack recently.

@nichoth
Copy link

nichoth commented Aug 18, 2017

I would factor things out in a generic way. I like to treat nested state objects as sort of black boxes. I don't think this makes the code smaller overall, which I think is good, as long as it's dry.

function Count () { return { realCount: 0 } }
Count.add = function (state, n) { return { realCount: state.realCount + n } }
Count.subtract = function (state, n) { return { realCount: state.realCount - n } }

var app = App({
    state: { count: Count() },
    actions: {
        add: (state, actions, n) => ({ count: Count.add(state.count, n) })
    },
    view: ...
})

The app functions only know about 1 level of state, and actions on nested state are delegated to children.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 18, 2017

@leeoniya I've used a HOF like this in the before.

function scopeActions(scope, withActions) {
  return Object.keys(withActions).reduce(
    (result, key) => ({
      ...result,
      [key]: (state, actions, data) => {
        return {
          [scope]: {
            ...state[scope],
            ...withActions[key](state, actions, data)
          }
        }
      }
    }),
    {}
  )
}

Using something like that mixed up with my favorite flavor of pick, lenses, getter/setter or my own custom abomination and I need nothing more.

@jorgebucaran jorgebucaran added discussion question How do I do that? labels Aug 18, 2017
@zaceno
Copy link
Contributor

zaceno commented Aug 18, 2017

@leeoniya, yes you're describing a problem experienced by many hyperapp-users (...not everyone though -- it's a subjective problem I think).

Plenty of patterns have been suggested, but everyone has their own preference, so there hasn't been enough of a consensus to warrant inclusion in core. Patterns suggested have range from small homemade helper code like @jbucarans suggestion above, to using libs like ramda's assocPath, lenses and suchs, and on to more elaborate (and perhaps "invasive") approaches.

My own solution, the one @andyrj mentioned above: https://github.com/zaceno/hyperapp-partial , probably belongs in the latter category. And it's quite fresh so while I'm happy for feedback I can't recommend it for real-world use yet. No docs yet either, but there's a codepen example in the readme demonstrating how it can help.

you have to repeat the state structure in multiple places

My "partials" leverage the mixin capability of hyperapp, to add the same state-structure multiple times under different namespaces. However that's statically defined at startup. I don't yet have a solution for dynamically creating/destroying instances of state-bundles. It's something I'm thinking about. An OOP-hybrid approach like the one @nichoth suggested seems quite sensible for that.

@leeoniya
Copy link
Author

leeoniya commented Aug 18, 2017

thanks for the feedback!

i know you guys are focused on a "tiny" and "ultra-simple" core, but the majority of non-trivial apps will need to invent a solution for this. a tiny core doesnt mean much if the user's app code then needs to grow exponentially to offset that simplicity. i suspect many would rather have a heavier core (even 2k, gasp!) and a much simpler and terser userland.

hopefully by 1.0 you guys have a single anointed solution, since hyperapp takes it upon itself to provide managed state.

/$0.02

@zaceno
Copy link
Contributor

zaceno commented Aug 18, 2017

@leeoniya You make a good point!

Still, I think the reason hyperapp hasn't built in a solution for this yet is lack of consensus, and not (purely) a focus on minimal core.

My prediction: if and when hyperapp goes more mainstream we'll start seeing some organic consensus forming in the shape of "technology stacks" (like "React + Redux"). At that point, we may well decide to build in the majority solution to avoid fragmentation in the community. At that point, I don't think bundle size will stand in the way.

Don't think it will happen by 1.0 though :( Perphaps 2.0 :)

@nichoth
Copy link

nichoth commented Aug 18, 2017

@leeoniya I think it's hard to get a feel for this because the example counter applications are so simple that they are already clear, and don't need any refactoring. Updating a large nested object is not a problem so much, because there are many utilities and patterns that will solve that. To me the underlying real problem is making the logic throughout the application clear and dry, encapsulating functionality in a good way. At that level it looks like a discussion of app design patterns, and finding the right pattern for your domain. But I could be missing the question here.

@leeoniya
Copy link
Author

"All problems in computer science can be solved by another level of abstraction...except for the problem of too many levels of abstraction." [1]

:D

[1] https://en.wikipedia.org/wiki/Indirection

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

@leeoniya Thank you for kickstarting this discussion. We've been exploring different solutions to this problem for a while, but have't settle on a particular approach yet. I am swaying towards scoped mixins, but that too is not without its cons.

Scoped mixins are those whose state and actions are nested under a namespace. We can figure out the namespace when you pass the mixin to the app call.

Currently, mixins are passed via an array, so we can't guess the namespace, but we could change so that mixins are passed via an object.


Going over your example again.

state: {
  count: {
    realCount: 0
  }
}
actions: {
  up(){
    return { realCount: count.realCount - 1 } // This won't work!
  }
}

What do you propose here? Clearly an action written just like that it's not possible to implement, because it's very hard to guess that realCount is a property in state.count.

But what if the following did work?

actions: {
  count: {
    up(){
      return { realCount: count.realCount - 1 } // This could work someday!
    }
  }
}

@leeoniya
Copy link
Author

leeoniya commented Aug 19, 2017

i think that actions could be passed a cursor creator to acquire a partial state and update the state through those cursors. this would solve the problem of destructuring being too simplistic and not having to recreate the full state path. some iteration on:

actions: {
  up(getCursor) {
    var partial = getCursor('count.realCount');
    partial.set(partial.get() - 1);
  }
}

you can also return the cursor itself after the update is captured and have hyperapp use the cursor's knowledge of the path to update the shared state at the right place. returning an array of updated cursors would work too.

actions: {
  up(getCursor) {
    var partial = getCursor('count.realCount');
    return partial.set(partial.get() - 1);
  }
}

should be pretty trivial to implement. all the cursor needs to do is temp hold a reference to the parent.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

@leeoniya What is your feedback about using the already existing namespace feature to scope action state updates, etc.?

actions: {
  count: {
    up(){
      return { realCount: count.realCount - 1 } 
      // This could work because we know `up` is inside `count`.
    }
  }
}

@leeoniya
Copy link
Author

leeoniya commented Aug 19, 2017

i'm not all that familiar with hyperapp, but by the sounds of it, would you be artificially restricting actions to only act upon (read/update) a specific node? if so, it seems much more restrictive than being able to acquire, read and update multiple disjoint places in the state.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

@leeoniya would you be artificially restricting actions to only act upon (read/update) within a specific node?

Yes, that's the problem with that.


So, you are basically proposing hyperapp supports this out of the box.

actions: {
  up(state) {
    return R.over(
      R.lensPath(["count", "realCount"]),
      value => value + 1,
      state
    )
  }
}

@leeoniya
Copy link
Author

yeah, plus returning an array of those.

though in your example you're additionally re-creating the setter on every call as well as the array for the path (perf). also ramda has a lot of issues open regarding slow perf. dunno if it applies to these functions specifically and any of their deps. also dunno how heavy the R.over & R.lensPath are.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

@leeoniya What if this was possible?

actions: {
  up(state) {
    return state("count.realCount", state => state + 1)
  }
}
actions: {
  up(state, actions) {
    return actions.bop("count.realCount", state => state + 1)
  }
}
const { bop } from "mmm"

// ...

actions: {
  up(state, actions) {
    return bop(state, "count.realCount", state => state + 1)
  }
}

@leeoniya
Copy link
Author

i'll let you guys hash out what you feel is best. i do think being able to return multiple disjoint updates (as an array) is important. it's probably faster to have the path as an array rather than having to run .split(".") on it anyways to get the same.

@jorgebucaran
Copy link
Owner

i do think being able to return multiple disjoint updates (as an array) is important.

What does this mean?

@leeoniya
Copy link
Author

What does this mean?

return [
  state("count.realCount", state => state + 1),
  state("some.other.thing", state => state + "foo"),
];

@jorgebucaran
Copy link
Owner

@leeoniya That'd be cool... 😄

How do you guys feel about built-in magic like this?

/cc @zaceno @matejmazur @lukejacksonn

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

The problem I have with supporting this out-of-the box is that then you'd feel discouraged to use Hyperapp with any of the other ton of libraries out there that help with this sort of thing, like Ramda.

Why not create your own "magic" function based on Ramda, etc., to help yourself? 🤔

We could provide an "official" package to help with this sort of thing too, but then I might as well include it into core...

In the future, I'd love to have a "hyperapp grimoire" that explains all this magical patterns and gives you the recipes, instead of distributing any "official" stuff on npm. 🌿🌱

@leeoniya
Copy link
Author

leeoniya commented Aug 19, 2017

sure, this doesnt really solve the full issue - that is, the actions needing to know the state hierarchy. unless you can pass a lens or a set of lenses into the action, all this solution helps is decrease the verbosity a bit.

@zaceno
Copy link
Contributor

zaceno commented Aug 19, 2017

@jbucaran, I would like something like that in hyperapp!

I was thinking maybe we could use thunks here too. Like what if the update function took a second argument indicating the scope in the state tree.

And perhaps rather than handling returned arrays like @leeoniya suggested, we already support calling update multiple times in an action.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

@leeoniya Exactly. For that I'd start with the scopeAction function I posted above.

We considered making actions scoped to their namespace by default early in the story, but it was too restrictive as you well pointed out. That's why it never made it to core.

So, what would be the most natural thing to do then that doesn't feel too forced or too restrictive? I can totally see that landing to core someday, but we need to answer that question first.

@leeoniya
Copy link
Author

leeoniya commented Aug 19, 2017

i guess that's the trade-off with single-state/unscoped actions: full visibility & needing to to specify full scope everywhere. or scoped actions and inability to update anything outside of their scope. i dont see a way to have your cake and eat it too, all in the same construct. the natural compromise seems to be just reducing the verbosity of attaining scope, so some form of cursor/lenses, but leave the actions unscoped.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

@zaceno Reusing thunks for this is a possibility, but then we'd end up using thunks so much that we might as well rewrite actions so that they all return a thunk. I feel that overloading thunks for this would end up breaking the balance between action/reducers and action/thunks.

TBH I still feel the most natural thing to do here is to go with scoped actions. So, perhaps we just need come up with a way to not make them as restrictive as Leon said.

If we can answer this question, how to enable the scope thing for some actions and not for others, we might just win the GOT. 😉

@leeoniya
Copy link
Author

i would find it quite confusing to have scoped actions that could also break out of their scopes. in addition, the scopes as you've shown then need to duplicate the state structure, which i don't find particularly fun.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

I am curious then, what is the ideal "syntax" to update nested state? Assume there is no hyperapp or that we had no restrictions. 🦄🌈

You posted some code:

const x = ({a.b.c}) => c.foo;

But that's not JavaScript... 😉

If the best we can do is cursors, lenses, etc., then IMO it'd best to leave it to a third party and not provide it out of the box.

@leeoniya
Copy link
Author

leeoniya commented Aug 19, 2017

But that's not JavaScript.

sadly no. but even if it was, that would only accommodate a single scope update. that's the problem with destructuring an argument and returning a single object. i think lenses are the best that can be done here with current technology. if you don't want to bake in a lens implementation, you could maybe do what i do to handle flyd streams in domvm [1]. create an an interface to plug in BYO lens implementations.

domvm.config({
  stream: {
    is:     s => flyd.isStream(s),
    val:    s => s(),
    sub:    (s, fn) => flyd.on(fn, s),
    unsub:  s => s.end(true),
  }
});

as for the exact conventions you want to use in hyperapp, you've got some decent opinions in this thread.

[1] http://leeoniya.github.io/domvm/demos/playground/#streams

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

Okay, I think I have an idea... 💡

actions: {
  up(state, actions) {
    return ["count", "realCount", state => state + 1]
  }
}

or

actions: {
  upAndMore(state, actions) {
    return [
      ["count", "realCount", state => state + 1]
      ["some", "more", "stuff", state => state + "foo"]
    ]
  }
}

Definitely an improvement of the weird state(path, fn) syntax a few posts above and a possibility now that the state is POJO.

Also, this might be is possible to implement outside core as a mixin using thunks and resolve.

@MatejBransky
Copy link

I like the solution with cursors/lenses. It could be outside of the core. ..just an helper. I use Ramda's lenses which are for me good enough. And I don't want scoped actions to be built-in. It should be just an mixin.

@leeoniya
Copy link
Author

leeoniya commented Aug 19, 2017

2 years ago i would have said that looks perfect, but i've come to appreciate understanding the intent of code without having to read the docs. POJS are nice, but can be hard to grok when used as essentially a nameless RPC.

this would be clearer:

return [
   state("count", "realCount", realCount => realCount + 1),
   state("some", "more", "stuff", stuff => stuff + "foo"),
];

but of course having state be a function is not great. state.update(...) has its own issues of collisions / pollution. update(state, ...) where update is passed in could work, or update(...) similar to https://github.com/hyperapp/hyperapp/blob/master/docs/actions.md#thunks

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 19, 2017

@leeoniya But here is the other thing. The above array jollification can be implemented outside core.

That's worth points. 💯

but can be hard to grok when used as essentially a nameless RPC.

Yes, that's true. Do you think an object is better? This decision is left to the author of the mixin, but one thing I like about using arrays is the proposed syntax looks a lot like Ramda. We are not inventing new stuff.

@ludoviclafole
Copy link

ludoviclafole commented Aug 19, 2017

I've made a quick example base on the propositions of @jbucaran
https://github.com/NoobLad/hyperapp-deepupdate

Completly open to issues/propositions etc...

we could add an object notation :

actions: {
  increment() {
    return deepState({
      "count.realCount"(state) {
        return state + 1
      }
    })
  }
}

Even if it's not used for any projects it could be used as a playground.

I can't say that it adds much when using libraries such as lodash/fp ramda qim etc...
A lodash example :

actions: {
  increment(state) {
    return compose(
      update('count.realCount',  v => v + 1),
    )(state)
  }
}

ramda :

actions: {
  increment(state) {
    const countLens = R.lensPath(["count", "realCount"])
    return R.compose(
      R.over(countLens,  v => v + 1),
    )(state)
  }
}

@jorgebucaran
Copy link
Owner

@NoobLad Very nice! 🙌👏👏

I'll have a deeper look when I implement my own which I hope to add to the docs to illustrate the power of thunks and resolve.

@alber70g
Copy link
Contributor

alber70g commented Aug 22, 2017

I've created something that allows for actions to do deep updates based on dot.notation. I'm not a 100% sure if it's something that should be in this issue, but in the spirit of DRY I think it's a good place ;)

Let's say you have this action:

actions: {
  setDeepProp: () => ({ 'set.deep.prop': 'value' })
}

results in a state:

{ set: { deep: { prop: 'value' } } }

Where as you can use ... 'operator' to set a deep object instead of a property

actions: {
  setDeepObject: () => ({ '...set.deep': { secondProp: 'value' } })
}

results in this state (taking the previous state as existing state)

{ set: { deep: { prop: 'value', secondProp: 'value' } } }

if you'd omit the ... spread operator in setDeepObject, it'll overwrite the property and not spread it:

{ set: { deep: { secondProp: 'value' } } }

I'm using an event for this, to 'reduce' the state to its intended state.

events: {
    update(state, actions, newState) {
      let intermediateNewState = { ...newState };
      for (const property in newState) {
        if (property.split('.').length > 1) {
          intermediateNewState = upsert(newState, property, newState[property]);
          delete intermediateNewState[property];
        }
      }
      function upsert(object, propertyPath, value) {
        function rec(objectTail, propertyPathTail, spread) {
          let [ head, ...tail ] = propertyPathTail.split('.');
          tail = tail.join('.');
      
          if (typeof objectTail[head] !== 'object') {
            objectTail[head] = {};
          }
      
          if (tail) {
            objectTail[head] = objectTail[head] || {};
            objectTail[head] = rec(objectTail[head], tail, spread);
            return objectTail;
          } else {
            objectTail[head] = spread ? { ...objectTail[head], ...value } : value;
            return objectTail;
          }
        }
        const spread = propertyPath.startsWith('...');
        return rec(object, spread ? propertyPath.slice(3) : propertyPath, spread);
      }
      return intermediateNewState;
    },
}

@lukejacksonn
Copy link
Contributor

lukejacksonn commented Aug 27, 2017

I am late to this party and only have one comment really..

but of course having state be a function is not great.

@leeoniya why is this so? I only asked because I was experimenting with this idea https://codepen.io/lukejacksonn/pen/weLRQL

I like the idea of:

setDeepProp: () => ({ 'set.deep.prop': 'value' })

or something like, as @alber70g suggested/implemented. Then again this really just building in lenses, essentially Ramda's assocPath right?

@jorgebucaran
Copy link
Owner

@lukejacksonn All of these ideas are cool, outside core! I like the one I proposed, but I see the good in @alber70g and @NoobLad ideas too.

was there a change to picodom's API?

I don't recall any big change, what's the problem?

@lukejacksonn
Copy link
Contributor

Not trying to push them into core! Just browsing for patterns with potential :) The picodom issue on the codepen was the a change in the order of patch inputs. All fixed now.

@jorgebucaran
Copy link
Owner

@lukejacksonn Geez, you are totally right. How could I miss that. My bad. 🙇

@jorgebucaran
Copy link
Owner

jorgebucaran commented Aug 28, 2017

Thank you @leeoniya for bringing this up and @alber70g, @NoobLad and everyone else that contributed to the discussion.

Here is my roadmap to close here.

return [
  ["count", "realCount", state => state + 1]
  ["some", "more", "stuff", state => state + "foo"]
]
  • Implement minimal deep state set/get using custom function.
app({
  state: easyState({
    count: {
      realCount: 0
    }
  }),
  actions: {
    count: {
      up(state) {
        return state("count", "realCount", realCount => realCount + 1)
      }
    }
  }
})
  • Implement immutable "magic state" based on Proxy.
app({
  state: magicState({
    count: {
      realCount: 0
    }
  }),
  actions: {
    count: {
      up(state) {
         state.count.realCount = state.count.realCount + 1
      }
    }
  }
})

@MatejBransky
Copy link

@jbucaran Could you send an example of the proxy version? I'm just curious how it works.

@jorgebucaran
Copy link
Owner

@matejmazur Sure, when it's done! 👋😄

@MatejBransky
Copy link

MatejBransky commented Aug 28, 2017

Just an another example of helper using Ramda (with relatively hardcore usage of Ramda 😄 ):

const set = (...changes) => (state, actions, data) =>
  R.pipe(
    ...R.map(
      change =>
        R.over(R.lensPath(R.dropLast(1, change)), previous =>
          R.last(change)(previous, { state: R.__, actions, data })
        ),
      changes
    )
  )(state);

Usage: set(...path, fn) - fn arguments: (previousValue, { state, actions, data })

  actions: {
    clearGame: set(
      ["a1", "b1", "c2", v => v + 1],
      ["a1", "b2", (v, { data }) => v - data],
      ["a2", "b3", () => true],
      [...path, (_, { state }) => !state.a3]
    )
  }

@lukejacksonn
Copy link
Contributor

Relatively hardcore but very cool at the same time 😎💯

@lukejacksonn
Copy link
Contributor

lukejacksonn commented Sep 2, 2017

@zaceno I've got a feeling that @matejmazur might have come up with a solution for #346 (comment) with his last comment.. or am I missing something 🤔 🔍

@garuse
Copy link

garuse commented Sep 2, 2017

My personal preference would be to access the keys with a dot notation instead of an array of keys. If some solution for this can be baked directly into hyperapp it would be very nice.

Currently for deep updates I am using an action like this:

actions: {
  deepUpdate(state, actions, [key, fn]) {
    key = key.split ? key.split('.') : key
    if ( key[0] !== 'state') key.unshift('state')
    let obj = { [key.pop()]: fn(eval(key)) }
    while (key.length) obj = { [key.pop()]: obj }
    return obj.state
  },
...

that can be used as:

actions.deepUpdate(["state.a.b.c.deepKey", v => v + 1])
actions.deepUpdate(["a.b.c.deepKey", v => v + 1])
// ugly :)
actions.deepUpdate([["a", "b", "c", "deepKey"], v => v + 1])

@zaceno
Copy link
Contributor

zaceno commented Sep 2, 2017

@lukejacksonn Well no, you're right, that's a solution for sure. And @okwolf 's example with the object-spread-operator is another option. There are plenty of ways :) but that wasn't exactly my point.

I meant more: is there a nice, reasonably concise & readable way to write deep-state-modifying actions in a safe, side-effect free way, without resorting to 3d party libraries or transpiling non-standard syntax?

...And I don't think there is! So that's why I brought it up. I think it's indicative of a fundamental problem in hyperapp which is: either you make your peace with mutating state directly (as I have... I know, shame on me ;) ), or you reach for a third party library. (or write your own helper library)

Technically, hyperapp could make it safe to mutate state props directly and return the mutated state, if it just made sure to provide each action (and now each reducer) with a deep clone of the state. It wouldn't be complicated code-wise (JSON.parse(JSON.stringify(state)) works), but performance would suffer terribly...(I think?)

@SkaterDad
Copy link
Contributor

SkaterDad commented Sep 2, 2017 via email

@zaceno
Copy link
Contributor

zaceno commented Sep 2, 2017

@SkaterDad good point about flattening state! (I know... I wrote hyperapp-partial to make nested state easier to work with, but still ;) )

Because none of this is a problem for shallow state modifications. (Neither syntax, nor mutating state)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion question How do I do that?
Projects
None yet
Development

No branches or pull requests