jorgebucaran / hyperapp

1kB-ish JavaScript framework for building hypertext applications
MIT License
19.07k stars 779 forks source link

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

Closed leeoniya closed 7 years ago

leeoniya commented 7 years ago

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 commented 7 years ago

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

nichoth commented 7 years ago

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 commented 7 years ago

@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.

zaceno commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

"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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@leeoniya What if this was possible?

1.

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

2.

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

3.

const { bop } from "mmm"

// ...

actions: {
  up(state, actions) {
    return bop(state, "count.realCount", state => state + 1)
  }
}
leeoniya commented 7 years ago

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 commented 7 years ago

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

What does this mean?

leeoniya commented 7 years ago

What does this mean?

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

@leeoniya That'd be cool... πŸ˜„

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

/cc @zaceno @MatejMazur @lukejacksonn

jorgebucaran commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@lukejacksonn Geez, you are totally right. How could I miss that. My bad. πŸ™‡

jorgebucaran commented 7 years ago

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"]
]
MatejBransky commented 7 years ago

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

jorgebucaran commented 7 years ago

@MatejMazur Sure, when it's done! πŸ‘‹πŸ˜„

MatejBransky commented 7 years ago

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 commented 7 years ago

Relatively hardcore but very cool at the same time πŸ˜ŽπŸ’―

lukejacksonn commented 7 years ago

@zaceno I've got a feeling that @MatejMazur might have come up with a solution for https://github.com/hyperapp/hyperapp/issues/346#issuecomment-326552693 with his last comment.. or am I missing something πŸ€” πŸ”

garuse commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

The same concern shows up in elm discussions too, since the updates to nested records happen in a similar way to the example above that uses object spread syntax.

The advice often given is to flatten your state when practical, to have less nesting.

Another thing I saw recommended was to make helper functions for updating the specific nested object, though this is really just moving the boilerplate out of sight.

I've been using the object spread syntax so far, and find it fairly annoying, but... It works and doesn't require strings and extenal libs (besides Babel of course). Haven't looked at how ugly the transpiled code is, though.

On Sat, Sep 2, 2017, 5:23 PM Zacharias Enochsson notifications@github.com wrote:

@lukejacksonn https://github.com/lukejacksonn Well no, you're right, that's a solution for sure. And @okwolf https://github.com/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.

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?)

β€” You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hyperapp/hyperapp/issues/333#issuecomment-326772415, or mute the thread https://github.com/notifications/unsubscribe-auth/AMpDr55YVAAaUdxXrg4k4O3cHyEbaz9xks5sedVagaJpZM4O72wx .

zaceno commented 7 years ago

@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)