Forbid lazy state return.

[jorgebucaran]

Even though the following "kind" of action is frowned upon, we could go one step further and ignore state updates (as well as re-renders).

myAction(state) {
  state.prop = 1
  return state
}

The change requires we simply check if the appState === newState and return early.

/cc @naugtur @zaceno @lukejacksonn

[alber70g]

There are two ways, either check it like you said or call the action with a new state, to begin with. I think it hurts performance because passing a new state when the action only returns a partial state, makes the passed state useless.

[zaceno]

To clarify: is the reason this is frowned upon, because we are mutating the state?

I'm concerned that if we simply ignore actions which return the same mutated objects, that means that subsequent actions would recieve the same, but now mutated, object reference. That would make things terribly hard to debug.

At least by accepting the mutated state-object as return value, we're ensuring that the next action called will get a separate instance.

Or am I misunderstanding something?

[jorgebucaran]

I'm concerned that if we simply ignore actions which return the same mutated objects, that means that subsequent actions would recieve the same, but now mutated, object reference.

No, they will not because we will skip this part.

//
// If newState points to the same object as appState then...
//
if (appState === newState) return appState
...
appState = newState

[zaceno]

right but appState was mutated inside the action?

[jorgebucaran]

@zaceno You mean if appState was mutated and not returned?

In that case, the check would also fail because we check whether the return value of the action is null or not.

But you are right, we would ultimately return appState to the caller (the most logical thing to do anyway).

[jorgebucaran]

FYI okwolf/hyperapp-logger#3

[zaceno]

@jbucaran IIRC we only create new state instances when we merge the returned value from an action. So if we for whatever reason don't merge after an action/update, then the state instance passed to any other action will be the same old isntance, potentially altered.

Whatever we do, I think we should take care that each update is called with a fresh instance.

[zaceno]

... unless that has terrible performance

[jorgebucaran]

IIRC we only create new state instances when we merge the returned value from an action.

Correct.

So if we for whatever reason don't merge after an action/update, then the state instance passed to any other action will be the same old instance, potentially altered.

We only merge if the new state is != null && != false.

if (withState && (withState = emit("update", merge(appState, withState)))) {
  requestRender((appState = withState))
}

The proposal here was to make that this:

if (
  withState && 
  withState !== appState && 
  (withState = emit("update", merge(appState, withState)))) {
  requestRender((appState = withState))
}

Whatever we do, I think we should take care that each update is called with a fresh instance.

Sorry, I don't understand this part. The internal update function is called with whatever you passed to it.

update("hello") // Wut? :)

[jorgebucaran]

By the way, I am okay if this doesn't go through. I am not strongly in favor or against this proposal! 😄

[zaceno]

Yeah I understand the change you're proposing, I don't think I'm explaining the problem I see with it well enough though. :-/

I mean: say you call that action in your example. It means there will be no render after. So the view will not change. Say another action is called (from the unchanged view) which counts on prop NOT being 1 (because the view made it available). It may use prop in some calculation but prop WILL be one because we allowed the previous action to mutate the current app state without causing a rerender.

At least with the current solution, what you expect to work works. Even though it's not the ideal way.

[naugtur]

I think I understand the concern described by @zaceno and it's more-or-less why I initially suggested that if mutating state and returning it from action instead of creating new is not ok, we should throw if withState === appState.

No matter what the reaction to them being equal is going to be, it's a leaky abstraction, because the references inside are not checked and it's easy to mutate the state even unintentionally, since the actions are not fed a copy of the state, but the original one.
The perfect (but not really performant) solution would be to deep clone state for every action.

Ignoring the update doesn't seem to be the right way to let developer know they shouldn't have mutated state.

[edit] This does look like a sufficient solution to the problem: https://github.com/okwolf/hyperapp-freeze
Everything else could be handled by documentation.

[okwolf]

@naugtur there are actually two separate issues here:

1. Mutating the current state from an action is a bad idea for many reasons, and should be avoided at all costs. My mixin can help with this.
2. Returning the current state from an action is not proper usage regardless of mutating or not. I could imagine Redux users doing this because they are used to having to return the current state when from their reducers in the case it should not change. My mixin will not help with this issue.

[jorgebucaran]

@okwolf More accurate is to say "from an action" in (1).

[okwolf]

@jbucaran edited my comment.

[naugtur]

@okwolf Agreed, (2) is a concern that'd be addressed by the change proposed here.

For better developer experience in case someone is mutating the state, logger could explicitly say if the state was updated or not as a result of the action. I think it'd be easy to add by listening to an event.

[jorgebucaran]

I think the correct thing to do here is to implement this proposal and produce an error message like @naugtur suggests in a future hyperapp dev build / distribution (which does not exist yet).

[zaceno]

Not sure I agree.

I see no problem with

myAction(state) {
  state.prop = 1
  return state
}

• as long as the state passed to the function always is a new instance. That's makes it ok to mutate and return it. It will not have side effects on anything else.

If we implement this proposal, what would be the right, side effect free way to write an action that needs to act on multiple properties deep in the state. How would you write this action in the proper way, with the proposed change:

myAction: state => {
  state.propA.propB.propC = 1
  state.propA.propD = 2
  return state
}

... ? The only way I can see would be to use a third party immutable.js , or make code more verbose with lots of Object.assign

If we always can guarantee that the state in to the action is a fresh state, there's no side effect issue writing actions the "improper way"

(I know I sound agitated, but I'm not. I welcome anyone explaining how I'm wrong :) )

[jorgebucaran]

@zaceno Copies are shallow and we can't efficiently satisfy your second issue unfortunately.

If we always can guarantee that the state in to the action is a fresh state, there's no side effect issue writing actions the "improper way".

I must admit I didn't think of that possibility. I still don't want to go with that because it would encourage you to write code this way:

state.prop = 1
return state

...and document that this is okay because we are giving you a new state.

There is nothing wrong with that approach if we pass a new object like you said though, but that would mean creating a new object, making a shallow copy of the current state and then merge with the previous state when the action returns inside update.

It seems like a lot of work for little gain.

[okwolf]

@jbucaran if we added a workaround for this use case and documented it as an acceptable pattern to use, I wouldn't feel comfortable calling Hyperapp mostly functional or based on The Elm Architecture anymore. If a users wants they can use existing solutions for deep cloning objects or immutable updates. You could even write a mixin to reduce the boilerplate of using that.

[okwolf]

If we implement this proposal, what would be the right, side effect free way to write an action that needs to act on multiple properties deep in the state. How would you write this action in the proper way, with the proposed change:

myAction: state => {
  state.propA.propB.propC = 1
  state.propA.propD = 2
  return state
}

@zaceno something along the lines of:

myAction: ({ propA }) => ({
  propA: {
    ...propA,
    propB: {
      ...propA.propB,
      propC: 1
    },
    propD: 2
  }
})

There are libraries that can help make this less verbose, but I don't think it's difficult to read personally.

[jorgebucaran]

And there is of course: #333 & #333 (comment) as well! 😄

[jamen]

I agree with @zaceno here. I thought of this as a feature of Hyperapp, because the mutation is safe unlike other systems in JavaScript, as far as I'm aware.

It is very useful in certain situations where returning a partial state is a pain, and it also allows you to skip creation of objects where there may be some performance impact. For example:

openPlaylist () {
  return { player: new Audio(...) }
}

// Should we create a new `Audio` here?
// Should we mutate & return partial state?
skipTrack(state) {
  state.player.skip()
}

To me it looks like what you would call safe mutation thanks to an evaluation strategy we are using. Handling the state before or after each action to make sure the next action is prepared.

I do not have much against mutation, I think it is a natural part of JavaScript. I'm against side effects caused by mutation. We should remember that we arenot in as well of a designed language like Elm

[jorgebucaran]

@jamen So what's your position on #346 (comment)? Do you mean you are in favor or against it?

[jamen]

@jbucaran I'm for keeping it. I think it's safe mutation by taking advantage of an evaluation strategy. I think mutation is a natural thing in JavaScript that many built-ins and libraries rely on. It's more useful to create safer environment to do it in Hyperapp than to simply disallow it, in my opinion.

[jorgebucaran]

@jamen But it's not really "safe", unless we implement something like what @zaceno proposed.

Keep in mind #346 (comment) anyway.

[jamen]

@jbucaran I didn't know if it was or not, because I've never hit an issue with it yet.

To make it safe, that's what I'm referring to by an evaluation strategy. Like @zaceno describes a call by copy-restore or call by object-sharing. Not trying to overload this with information, just trying to point out there is safe ways to do this and it makes hyperapp quite useful imo.

[jorgebucaran]

@jamen And can this evaluation strategy be applied without changes in core?

[jamen]

Maybe with events.resolve as a mixin? I'm not 100% sure. But for the core, I will have to experiment to see. Would be more useful out-of-the-box if the changes are light, you think?

[jorgebucaran]

@jamen I was just curious. Remember that the goal this time was to decide whether we want to deliberately forbid lazy state return do nothing.

It seems opinion on the issue is mixed, so I'm in favor of closing here and do nothing.

Hyperapp stance will continue to be "don't do it" or "do it at your own risk".

[jamen]

@jbucaran I see. Then my opinion would be to do nothing. But make it safer later (as I thought it already was)

[zaceno]

Oh... I get it now! Mutating state in props is unsafe because of the shallow merge. While each action gets a new state instance, the properties containing objects will not be fresh instances.

Ok, so the unsafeness of mutating deep state properties is a completely separate issue to what's being proposed here (because it would continue being unsafe, even if we did implement this).

...even so, I'm still not sure I like this proposal because it will be difficult to debug. I've written that sort of inappopriate actions in many places. This sort of "silent fail" would make refactoring harder. So if we eventually do take it in, it should be after we have a dev-build which can log warnings. Or failing that, make production builds throw errors terminating and providing stack traces.

@okwolf Thanks for giving that example! I was actually aware of the object-spread operator to use in this case, but for me personally, I don't consider it a solution as it is only a stage-3 proposal and not yet standard ecmascriot.

[jorgebucaran]

@jamen Yep, we are on sync now. 😉

And my opinion about that was expressed here.

tl;dr: This is a do-it-at-your-own-risk kind of "feature"; I won't force you to do things in a particular way (although you should), but I am not going to go "out of my way" to let you do it either (unless there's some trivial change that allows it).

[jorgebucaran]

Closing as wontfix. 😄

#346 (comment)