jorgebucaran / hyperapp

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

Allow a custom merge method #532

Closed titouancreach closed 6 years ago

titouancreach commented 6 years ago

Currently, when a action return a new state, this one is merged with the previous state. That make the object "state" to grow but never shrink. I would imagine this behavior to be configurable with a pure function. where the default would be:

const mergeState = (previousState, newState) => ({...previousState, ...newState});

To have the same behavior as redux for example, we can use this function

const mergeState = (previousState, newState) => newState;
jorgebucaran commented 6 years ago

@titouancreach How would that look in a simple app and why would I want my state to shrink?

titouancreach commented 6 years ago

The first example that would come to my mind would be: I want to create a simple router, I would be able to do:

import defaultStatePageFooBar from '...';

actions: {
  goToPageFooBar: () => state => defaultStatePageFooBar
}

I find the second mergeState more functional and more natural because you don't hide a "merge" to the user.

Sure you can trick with a nested state:

const state = {
  appstate: {
    // ...
  }
}

and in your action do:

import defaultStatePageFooBar from '...';

actions: {
  goToPageFooBar: () => state => ({appstate: defaultStatePageFooBar})
}

With the nested state, you're doing like redux with multiple reducers.

jorgebucaran commented 6 years ago

@tiaanduplessis And how would that look like on your app?

titouancreach commented 6 years ago

You specify the merge method in the constructor as an optional parameter:

app(state, actions, view, document.body, mergeState);

This allow to do awesome things. If you want to to prefix your state (for any reason) or whatever, you can use function composition:

mergeState = compose(setState, prefix, mySideEffectFunction, ...)

The user has now the full power of how its state is updated

whaaaley commented 6 years ago

I like the idea of non-merged state. I know merged state has it's own pros, but merging doesn't really help me with my own apps.

If I have some state like this and and I want to update the state.

// initial
{
  foo: 'bar',
  bar: 'baz'
}

// now i want this
{
  foo: 'ooga booga'
}

I won't be able to. I'd have to set bar to null myself to denote that it's "removed".

// initial
{
  foo: 'bar',
  bar: 'baz'
}

// now i want this
{
  foo: 'ooga booga',
  bar: null
}

And of course, as your state gets more complex, it's not as easy as setting a single prop to null.

It's not a "con" per se. It's just a way I would prefer to build my apps. Imo, when being able to replace a state object "wholesale" like this, it's easier to read and less code to write, both with actions and in views.

Right now a way around this is to just put everything one level deeper to avoid the merge.

When state wasn't immutable, I'd imagine leaving "removed" state as null would be a perf gain, as you wouldn't need to constantly, remove and re-create that prop, but now that everything is immutable, there's not anything to gain, perf wise.

jorgebucaran commented 6 years ago

@whaaaley Why do you want this { foo: 'ooga booga' }? 🤔

@titouancreach I think this feature request; how it works and what it does is clear by now, so the next step is justifying it, i.e., answering "why".

It's very unlikely I'll add something new to Hyperapp just because it's possible, so I want to know why would I want the ability to do this?

okwolf commented 6 years ago

The current state update approach in Hyperapp reminds me of updating records in Elm or Object.assign(state, { key: value })/{ ...state, key: value } from ES 6, but without the boilerplate of bringing in the previous state values.

This is by far the most common use case, and the only one I've used in reducers for Redux.

titouancreach commented 6 years ago

I understand the point of view. I think if we only "update" record (update value in) we should not be able to create new keys.

For example:

const state = {
  count: 0
};

const actions = {
  myAction: () => state => ({ foo: 'foo' });
};

Should not be possible. It can possibly be a typo

const actions = {
  myAction: () => state => ({ coount: 'foo' });
};

Then, I would namespace the actions:

const state = {
  counter: {
    value: 0 // since state can only be object
  }
};

const actions = {
  counter: {
    myAction: () => state => ({ value: state.value + 1 });
  }
};

In this case, it won't be possible to create a key a top level without the possibility to remove it. Actions are also more "local" since they work with a part of the state and not the full state. It will encourage to make more little actions.

whaaaley commented 6 years ago

Why do you want this { foo: 'ooga booga' }? 🤔

Maybe I'm listing out all my state in one of my slices. Of course I could add a condition to check for null/undefined here, but why bother if I could just update my state to not include those?

const list = state => {
  const result = []
  for (let key in state) {
    result.push(h('div', null, `${key}: ${state[key]}`))
  }
  return result
}

Maybe I just want to bulk update all of my state at once and get a fresh start where a few or none of my state props are the same, so my views don't get funky. Like when retrieving data from an endpoint or user input.

// i started with this
{
  foo: 'one',
  bar: 'two',
  baz: 'three'
}

// now i want this
{
  abc: 'seventy two',
  xyz: 'eighty nine'
}

Which could change the way the view is rendered.

if (state[key_based_off_of_user_input_here] && state[key_based_off_of_user_input_here] === 'two') {
  // do code
}

I've been meaning to create an example that better demonstrates this. The reason I haven't is because this is more of a preference rather than something that has a ton of functional gain, because like I said, you can just nest your "complex"/non-mergable objects one level deeper to avoid this. I just enjoy replacing state completely while developing to be honest. I do this in my test apps with my pocket project. If it doesn't get added I wouldn't be terribly heartbroken, but I would enjoy it if it did.

How would that look in a simple app and why would I want my state to shrink?

If your app is continuously adding dynamic state, which could be amplified if you're app is persisting state, your state tree could become very "bloated". Sort of like a very slow memory leak. I think this ties into what @titouancreach was saying about not being able to dynamically create new keys if you can't clean them up. However, I WOULD be heartbroken if we removed that functionality. 💔

vdsabev commented 6 years ago

Most people are used to spread-merging { ...previousState, ...newState } from Redux. I like the deep merge, but wanted the option to change the function as well, which is why I implemented it in Derpy.

I think if we keep adding parameters to app we might wake up with 7 or 9 someday soon 😆 How about something more future proof like:

app(state, actions, view, document.body, { merge: Object.assign });
jorgebucaran commented 6 years ago

I don't consider this to be remotely critical, but I would reconsider if it was. However, none of the answers has been able to justify adding this, so I am gonna go with @okwolf's and avoid us option paralysis.

This is by far the most common use case, and the only one I've used in reducers for Redux.

EDIT: If this turns out to be relevant to implementing something important in the future, I am happy to reopen and add it.