Logs the same object as prev and next state

[naugtur]

When an action is not creating a brand new object but extending the existing one before returning, logger will log the original reference instead of a copy/deep clone. The object gets updated before it's printed to the console and both prev and next logs are identical, which is misleading.

Possible fixes:

• deep clone the state object on prev before logging
• pause execution when logging to make up for console.log being actually asynchronous (probably a bad idea)
• documentation to clearly state this happens if you're lazy and don't implement actions with the assumption that the state is immutable.

[jorgebucaran]

@naugtur When an action is not creating a brand new object but extending the existing one before returning...

That is not a valid action:

actions: {
  badAndNaiveAction(state) {
    state.value = 1
    return state // Oh, no!
  }
}

If you are doing that, then you'll only get yourself in trouble, like I presume you are describing, but correct me if I am wrong!

I admit the docs should be more clear about this. Right now this is what they say:

Returning a partial state will update the state immediately and schedule a view re-render on the next repaint.

But it could add that you should always return a new state or partial state, and not mutate the state you were given and return it.

[naugtur]

Yes, I was being lazy. I've already fixed the function, but I do think logger, as a developer tool, should not create confusing output especially if it seems the developer is lazy or less skilled ;)

Alternatively, maybe it's worth spending a few bytes to add a check to hyperapp like this:

if ( newState === oldState ) { throw Error('actions must return new state') }

[jorgebucaran]

@naugtur Yes, I was being lazy.

It's okay, I am lazy too sometimes! My point was that if you were use actions incorrectly, then you should do it at your own risk.

But I would be happy to hear ideas about how the logger could help you if you are in a lazy mood though.

Alternatively, maybe it's worth spending a few bytes to add a check to hyperapp like this...

Sounds good to me. But we don't have to throw, simply skipping the update is a good compromise

We won't be able to be lazy though, are you sure? 😉🤔

At any rate, I'd be happy to add those extra bytes to core.

[okwolf]

@naugtur if you want mutating the state in place to be an error during development, consider using freeze: https://github.com/okwolf/hyperapp-freeze

[jorgebucaran]

@naugtur I closed #346 as wontfix. Should we do the same here @okwolf?

Or is there anything the logger can do for us?

[naugtur]

I suggest a warning if new state===oldstate with a link to explanation why would be best

[jorgebucaran]

@naugtur A warning belongs to a development build / distribution.

On the other hand... the logger is not really for production use.

[naugtur]

My point exactly. I'm configuring my build to skip the logger next week.

[jorgebucaran]

@naugtur Cool. How do you skip it?

[naugtur]

I'll try two options and see which I like better.
Use webpack's configuration for replacing modules or create a separate entry script with all the dev stuff enabled.

More likely the latter.

[okwolf]

@jbucaran this sounds like something that should be configured with options. My personal preference would be off-by-default, although I wonder if on-by-default would be better for newcomers to to Hyperapp who may not know this gotcha.

[okwolf]

@naugtur I like the idea of a "higher-order" app where dev is a composed of prod plus extra features instead of patching in two separate modules. I'm going to be writing some utils to help with this and might even propose API improvements to make it builtin later @jbucaran.

[jorgebucaran]

@okwolf his sounds like something that should be configured with options. My personal preference would be off-by-default

If you want to bake this into the logger, then please make it false by default. But I would prefer if the logger did not mind this. I think this kind of correctional behavior belongs in a dev build of hyperapp, not in a mixin.

And about a higher-order app, that sounds interesting!

[okwolf]

@naugtur take a look at #2 and let me know what you think.