andyrj / hyperapp-redux-devtools

hyperapp HOA to utilize redux-devtools-extension from hyperapp
MIT License
36 stars 5 forks source link

fix problem with nested state and actions #9

Closed yhirose closed 6 years ago

yhirose commented 6 years ago

I just found a problem with 'nested state' as bellow. ('nested state' is described here in the Hyperapp document.)

const state = {
  counter: {
    count: 0
  }
}

const actions = {
  counter: {
    down: value => state => ({ count: state.count - value }),
    up: value => state => ({ count: state.count + value })
  }
}

The PR fixes the problem. It recursively handles nested sync actions properly as the wireStateToActions does in Hyperapp itself. https://github.com/hyperapp/hyperapp/blob/master/src/index.js#L96-L124

andyrj commented 6 years ago

I need to give this a better look later but the code looks fine. I will merge as long as my local test setup works with it.

yhirose commented 6 years ago

Sounds good. One more note about my change is that copy and set functions are copied from the Hyperapp code. If the Hyperapp exposes the functions, we don't have to copy them.

andyrj commented 6 years ago

True, but I don't think the code duplication is to bad being a devtool.

andyrj commented 6 years ago

looks good, merging and will publish 1.1.4 to npm shortly

andyrj commented 6 years ago

I actually found a bug with this PR just after merging this heh... looking into it now.

edit: fixed and published 1.1.4 to npm

yhirose commented 6 years ago

Thanks for finding the bug and fixing it!

I have tested the version 1.1.4 with my test application and confirmed it works. 👍 (https://github.com/yhirose/hyperapp-typescript-electron-boilerplate/blob/master/src/render.tsx)

andyrj commented 6 years ago

@yhirose You are welcome. Sorry it took me a couple days to get around to giving this a proper review.