Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce fragment state / state slices. 🍰 #385

Merged
merged 13 commits into from
Oct 5, 2017
Merged

Conversation

jorgebucaran
Copy link
Owner

@jorgebucaran jorgebucaran commented Sep 22, 2017

Hi everyone! 👋

⚠️ This is a massive breaking change and the last one before 1.0. After this is merged and details are sorted, I'll publish 1.0 and then we can celebrate!

What's new?

This PR bids farewell to the following concepts/features:

and adds:

Fragmented State & Actions

Fragmented means your actions now receive a fragment (think slice 🍰) of the state tree that corresponds to the leaf/namespace in which they are declared.

actions: {
  hello(state) {
  // The state is the global `state`.
  },
  foo: {
    bar: { 
      howdy(state) {
      // The state is: `state[foo][bar]`
      }
    }
  }
}

With this feature, you can update deeply nested state more easily. Close #333.

For example, if you had the following state:

state: {
  foo: {
    bar: {
      value: 0,
      anotherValue: 1
    }
  }
}

...and wanted to update value, you'd end up updating an entire record (siblings too), since there is no way to single out value from a nested state tree.

In other words, you'd need to write something like this in order to be inmutable.

actions: {
  updateValue(state) {
    return {
      foo: {
        bar: {
          value: state.foo.bar.value + 1,
          anotherValue: state.foo.bar.anotherValue
        }
      }
    }
  }
}

With fragmented State & Actions, it's possible to update value immutably and easily:

state: {
  foo: {
    bar: {
      value: 0,
      anotherValue: 1
    }
  }
}

And the corresponding action:

actions: {
  foo: {
    bar: {
      updateValue(state) {
        // State is `state[foo][bar]`
        return { value: state.value + 1 }
      }
    }
  }
}

Here is contrived, but complete example using components.

const counter = {
  state: {
    value: 0
  },
  actions: {
    up(state, actions) {
      return { value: state.value + 1 }
    }
  }
}

function Counter(props) {
  return (
    <main>
      <h1>{props.value}</h1>
      <button onclick={props.up}>1UP</button>
    </main>
  )
}

app({
  state: {
    counter: counter.state
  },
  actions: {
    counter: counter.actions
  },
  view: (state, actions) => (
    <Counter value={state.counter.value} up={actions.counter.up} />
  )
})

Life after events

What's life going to look like after events.load is gone?

const actions = app({})

// Register global DOM event listeners using document.addEventListener
// Do things you would normally use events.load for here.

actions.doThat()

Hydration by default

Before to enable hydration you'd return a hydrated node from events.load. Now that events are no more, you just sit down and do nothing. We hydrate your stuff for you.

How? Well, we just check if there are children in your root and we assume you rendered them on the server. If that's not the case you will need to provide a different root element that you own. 🎉

onremove with thunks

You don't have to remove elements inside onremove handlers by yourself now. Return a function from onremove to access a remover remove function and call it when you are done with your business. Close #357

function AnimatedDiv() {
  return <div onremove={element => 
     remove => fadeout(element).then(remove)}
  />
}

Life after Mixins

Mixins introduce implicit dependencies when abused, and they are easy to abuse.

Say you have two mixins, mixinA and mixinB. In one or more of mixinB's actions you call an action exposed by a mixinA. This is possible, because currently, you can call any action from within an action. After this PR, you will be able to call only actions inside the same namespace.

So, in this way, mixinB depends on mixinA. This is an implicit dependency. If you remove mixinA, mixinB will break. Not to mention, how hard it is to test and debug mixinB and how opaque this relation is.

But with fragmented state and actions, what harm can mixins cause? Good question! The answer is that we could actually keep them, since fragmented state and actions would make inter-mixin communication impossible.

const mixinA = {
  state: {
    A: {
      value: 0
    }
  },
  actions: {
    A: {
      setValue(state, actions, value) {
        return { value }
      }
    }
  }
}

app({
  mixins: [mixinA]
})

The problem with writing code this way is that authors are forced to define the mixin's namespace and nest their state, which is prone to name collisions and complicated state tree updates. It also means you will have to check mixinA docs or source code to figure out what was the namespace it uses.

Now without mixins, how do we encapsulate mixinA's state and actions, which is what we are really after anyway?

Easy!

const A = {
  state: {
    value: 0
  },
  actions: {
    setValue(state, actions, value) {
      return { value }
    }
  }
}

app({
  state: {
    A: A.state
  },
  actions: {
    A: A.actions
  }
})

@codecov
Copy link

codecov bot commented Sep 24, 2017

Codecov Report

Merging #385 into master will decrease coverage by 0.66%.
The diff coverage is 98.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
- Coverage     100%   99.33%   -0.67%     
==========================================
  Files           2        2              
  Lines         146      151       +5     
  Branches       46       44       -2     
==========================================
+ Hits          146      150       +4     
- Misses          0        1       +1
Impacted Files Coverage Δ
src/app.js 99.28% <98.21%> (-0.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e2dc03...2d0f87e. Read the comment docs.

@okwolf
Copy link
Contributor

okwolf commented Sep 24, 2017

@jorgebucaran I fixed all the tests. Coverage is almost complete again.

src/app.js Outdated
@@ -8,7 +8,7 @@ export function app(props, oldNode) {
var appActions = {}
var appState = props.state
var appView = props.view
var appSubs = props.subscriptions
var appSubs = props.subscriptions || []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the default there were errors leaving out subscriptions.

@okwolf
Copy link
Contributor

okwolf commented Sep 27, 2017

So to me, after this PR:

Hyperapp = Elm (same architecture) - Effects as Data (built-in support for avoiding side effects in app code) + auto Redux combineReducers (fragment state) + Redux Thunk (return functions from actions for deferred state updates) + Redux applyMiddleware (subscribe/respond to app lifecycle events (load/action/resolve/update) and add support for new action types)

okwolf
okwolf previously requested changes Sep 27, 2017
src/app.js Outdated
}

function getPath(path, source) {
return path.length === 0 ? source : getPath(path.slice(1), source[path[0]])
Copy link
Contributor

@okwolf okwolf Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgebucaran I think there's a bug here, or at least an inconsistency. Based on this code in setPath below(line 100):

source != null && name in source
  ? source[name]
  : path[1] >= 0 ? [] : {}

I would expect an update for a state fragment that doesn't exist yet to work, however this line will throw an error like cannot read property 'foo' of undefined from getPath. One possible fix would be to write:

return path.length === 0 ? source : getPath(path.slice(1), source[path[0]] || {})

instead. I came across the issue because the condition in setPath is currently not covered by the tests and I was trying to add coverage.

@okwolf okwolf dismissed their stale review September 27, 2017 07:09

new push

src/app.js Outdated
data
)
function getPath(path, source) {
return path.length === 0 ? source : getPath(path.slice(1), source[path[0]])
Copy link
Contributor

@okwolf okwolf Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgebucaran I think there's a bug here, or at least an inconsistency. Based on this code in setPath above (line 109):

source != null && name in source ? source[name] : {}

I would expect an update for a state fragment that doesn't exist yet to work, however this line will throw an error like cannot read property 'foo' of undefined from getPath. One possible fix would be to write:

return path.length === 0 ? source : getPath(path.slice(1), source[path[0]] || {})

instead. I came across the issue because the condition in setPath is currently not covered by the tests and I was trying to add coverage.

@SkaterDad
Copy link
Contributor

@jorgebucaran @okwolf
Can you clarify how the subscriptions are used? The name is pretty confusing, since it appears more like middleware/hooks. When I see "subscribe", I think of observables or attaching handlers to browser events.

Looking through the code in app.js, this is what I think they do:

subscriptions: [
  (state, actions) => {
    // this gets called on app load
    // replacement for "load" event
    return ({name, data}) => {
      // this gets called before an action is performed
      // replacement for "action" event
      return (result) => {
        // this gets called with the result of an action
        // replacement for "resolve" event
      }
    }
  }
]

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Sep 27, 2017

@SkaterDad Edited my original post. The name is going to be hooks.

Looking through the code in app.js, this is what I think they do...

That is 💯 exactly what they do.

@SkaterDad
Copy link
Contributor

Thanks for clarifying!

@lukejacksonn
Copy link
Contributor

@jorgebucaran out of curiosity.. why hooks and not subscriptions like elm?

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Sep 27, 2017

@lukejacksonn Because while the concept is "similar" to Elm's subscriptions, I've come to learn that what we are doing is different.

Our subscriptions are a simplified one-size-fits-it-all Elm's init function, subscriptions and our former events (which are more like "hooks").

Unlike Elm, we are not inside a bubble (I mean that in a good way) and can simply use the platform directly; use addEventListener to subscribe to GlobalEvents, use WebSocket, setTimeout, setInterval, fetch, etc. We don't need a special module; we have no use for this abstraction.

We don't have ports. In Elm, subscriptions are also the high-level interface to the lower level ports.

Finally, hooks is shorter than the longer subscriptions, and I know this is my personal opinion, but I feel the term hooks is more familiar and casual. 💯

@VinSpee
Copy link

VinSpee commented Sep 27, 2017

Have you considered cutting an alpha of this so we can easily use it in codepen/codesandbox/whatever to play around with? I've been copying and pasting so far 🤡

@lukejacksonn
Copy link
Contributor

lukejacksonn commented Oct 5, 2017

What @VinSpee said.. awesome demo @SkaterDad 💯 and great question too! We are all in the same boat here, testing out new patterns. What I would have suggested 12 hours ago would have been something similar to what was suggested, using hooks. You have prompted me to think about it further now.. and routers in general (especially a component based routers).

If hooks disappear and app returns actions then you can change the apps state from outside of app by calling actions. In this particular case we want to subscribe to window.popstate events and call router.go every time one is triggered. How about something like..

const call = app({
  state: {
    backpack: {
      items: []
    },
    router: {
      path: '',
      fetching: false,
    }
  }, 
  actions:  {
    backpack: {
      fill: (state, actions, items) => ({ items })
    },
    router: {
      fetching: (_, __, fetching) => ({ fetching }),
      go: (state, actions, destination) => {
        const { request, callback } = routes[destination]
        request && actions.router.fetching(true) && fetch(request)
          .then(data => callback(data) && actions.router.fetching(false))
        history.pushState(null, null, destination)
        return { path: destination }
      }  
    }
  },
  view: (state, actions) =>
    h('main', {}, !state.router.fetching && [
      state.router.url === '/'
        ? h('h1', { onclick: actions.router.go('/adventure') }, 'Go on an adventure..'),
        : backpack.items.map(item => h('li', {}, item)),
    ])
})

const routes = {
  '/adventure': {
    request: '/api/data',
    callback: call.backpack.fill
  }
}

const route = e => call.router.go(location.pathname)
window.onpopstate = route
route()

This is just off the top of my head late night pseudo code.. but hopefully you get the idea 😅

@okwolf
Copy link
Contributor

okwolf commented Oct 5, 2017

I wish we had kept this branch/PR focused on the original topic of state slices/fragments 🍰 so we could merge that update in separately while still discussing the merits of the other updates independently.

@jorgebucaran
Copy link
Owner Author

@okwolf Sorry, man. I agree, but now it's too late! :)

@okwolf
Copy link
Contributor

okwolf commented Oct 5, 2017

@jorgebucaran water under the bridge, my friend. A lesson for future us ⏳

@IamCarbonMan
Copy link

IamCarbonMan commented Oct 14, 2017

So how does one implement the old events.action, events.update, etc? Those were removed and replaced with hooks, which were removed and replaced with HOAs, which were removed and replaced with... nothing?

@jorgebucaran
Copy link
Owner Author

@IamCarbonMan With HOAs.

HOAs were not removed, what we removed was some "special built-in HOA support" that let you write HOAs in a particular way that, after careful consideration, was not any better (in fact worse and less flexible) than the way you would write a higher order function of any function in JavaScript.

@IamCarbonMan
Copy link

@jorgebucaran so I'm guessing the suggested way to implement something like events.action is just to write an HOA that wraps each action with whatever logic you need?

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Oct 14, 2017

@IamCarbonMan Correct, that's the way. The logger is being rewritten like that and here is the PR okwolf/hyperapp-logger#7 with the changes.

Usage will be like this.

logger({
  // Logger pptions
})(app)({
  // App options
})

/ cc @okwolf

@IamCarbonMan
Copy link

IamCarbonMan commented Oct 14, 2017

That's actually really smart. Especially with the PoC terminal UI it really feels like Elm, you're slowly going from a virtual DOM library to a simple state machine that maps one piece of data (a JS object) to another (DOM or some other view). Really excited to see where this is going!

Any expectation for hyperapp/router to go this way?

@okwolf
Copy link
Contributor

okwolf commented Oct 14, 2017

@IamCarbonMan shameless plug for my hyperapp-middleware that you can use to build such HOAs. 😊 🔌

@FlorianWendelborn
Copy link

can you provide an example

I can't share the source code, but the app I'm writing is pretty complex since it needs to manage a lot of things like WebRTC, WebSockets, postMessage communication, localStorage, etc..

I need a reliable way to split code between multiple files and so far, mixins were perfect for this. I could call stuff like actions.message.send() from any mixin that needed to communicate via postMessage with a different iframe. I don't see any way to do this without basically using a giant hacky workaround that doesn't fit into the current hyperapp usage at all. It'd probably be easier to just switch to Preact and write a custom state management library that imitates hyperapp pre-merge at this point.

@zaceno
Copy link
Contributor

zaceno commented Oct 16, 2017

@dodekeract While i completely agree with the need for modules to be able to communicate beyond top-down, I disagree with the pattern of calling an action directly in another module (other than top down, which is ok), because of the dependency mess it can create. (Like what if you rename message module to com, how many places will you need to change that?).

This is why the custom events of yore were so valuable: They allowed dependency-free cross module communications.

Of course, you know your situation better than me -- but from what you've described and If I were you, I would rather integrate an event bus and refactor all actions.someothermodule.xxx(...) to emit calls. Not just for the sake of sticking with hyperapp, but more importantly to decouple my modules.

@FlorianWendelborn
Copy link

FlorianWendelborn commented Oct 16, 2017

@zaceno I'm not needlessly tying modules together. The whole app needs to work with all other parts of the app and there is no sensible way to do it differently, at least none that I'm aware of.

Converting actions to emit is, IMHO, exactly the same with a different implementation. It doesn't make them any less connected.

how many places will you need to change that

For actions.message.send, 19 places. It's one of the "core" actions I have that are needed to connect the apps across different iframes, and no, it can't be easily removed from hyperapp as it contains logic that accesses the state, and other actions. After all, the postMessage channel isn't trusted in my case, so there needs to be some crypto applied to it and I also have async/await support for postMessage actions.

dependency-free cross module communications

I completely agree that it would be better to have no dependencies between mixins, but honestly, that's unrealistic in my case. I outsource everything I can into as small/well-defined as helpful mixins. This works quite well for this particular project. The flexible mixin system is one of the reasons why I decided to upgrade hyperapp instead of porting the app to Preact when breaking updates came out. It was helpful, and still is (as I didn't upgrade yet, for obvious reasons).

@IamCarbonMan
Copy link

@dodekeract so if there was a way to continue using mixins, with the same API as before, using the latest hyperapp, would you be able to make use of that? It should be trivial to reimplement mixins using an HOA.

@FlorianWendelborn
Copy link

FlorianWendelborn commented Oct 16, 2017

@IamCarbonMan if there's something that passes the global actions/state to all actions then all problems I have with the recent changes should be solved, yes.

The thing I don't like about using a HOA for this is that dropping official support and outsourcing it means that it won't be considered for future API changes. So, it might become more and more hacky/impossible over time.

@IamCarbonMan
Copy link

IamCarbonMan commented Oct 16, 2017 via email

@FlorianWendelborn
Copy link

FlorianWendelborn commented Oct 16, 2017

@IamCarbonMan I'm pretty sure it's practically impossible to do this project in the new API. Of course, I could always completely circumvent all of hyperapp's state management solutions, but then I might as well write my own state manager. 🙂

stay on the old API

Well, for now that's the only thing I can do. It's not a long-term solution though and this'd probably lead to me having to hard-fork hyperapp. In that case, I'd most likely convert all my code to preact/redux instead of maintaining a fork.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Oct 17, 2017

@dodekeract The thing I don't like about using a HOA for this is that dropping official support and outsourcing it means that it won't be considered for future API changes.

This is incorrect. We didn't drop official HOA support, just for the same reason we can't drop official support for the if statement. You could also create HOAs before, we just never explored that path.


I am pretty sure that if you were using mixins for code organization, it should be relatively easy to port everything to the new API and I encourage you to try it. On the other hand, if you were abusing mixins to take advantage of implicit dependencies, then I am afraid you may need a HOA. 😉

I suggest you forget about HOAs and take this opportunity to improve your application architecture by adopting the new API.

@zaceno
Copy link
Contributor

zaceno commented Oct 17, 2017

@dodekeract I didn't meant to imply that you were carelessly creating a messy structure of mixins. Hope it didn't sound like that. I'm sure your design is carefully and well thought out.

There are principal differences between events and direct-action-calling. One of them is, that if the action you are calling goes a way (or is renamed), your whole app breaks then and there. If the event you are emitting no longer has listeners, you don't get the desired effect, but the rest of the app keeps running. Because emitting events creates no dependencies on listeners (and vice versa).

Mixins/modules communicating via events are loosely coupled.

The bottom line is the sameof course: your app doesn't work. In fact you might prefer your app to stop dead in its tracks any time something isn't as it should be (easier to catch and track down bugs).

But I prefer to have the loose coupling. To me it is in line with the idea of "optimize for deletablility" -- structure your app so you could take a big chunk out and rewrite it with minimal effect on the rest of the code.

@tunnckoCore
Copy link

tunnckoCore commented Oct 23, 2017

Sounds and looks pretty fantastic in first look. But while i'm reading about the case of @dodekeract i'm thinking that he have a right. It is interesting scenario. Both architectures have Pros and Cons.

And if you are thinking to fork.. you may consider contacting me. I had few implementations born around when Hyperapp was born. I was reimplementing such architectures/apis several times to found my vision, soon after that freactal popped up, which was exactly what i was thinking. (the tunnckoCoreLabs/elmaz#2 is final thought - it has event bus and central error handling, targets > edge12-14 browsers; and this is another one tunnckoCore/ideas#92) I'm trying to find time to release such library for few months, but i'm busy and sadly i messed my account and workflow totally recently :D

But in any way, @jorgebucaran great ideas is happening here. I'm just curious why you don't use picodom directly? And just wrap it which the state management?

jorgebucaran added a commit that referenced this pull request Jan 7, 2018
- Introduce fragment state & actions.
- Remove mixins, events (hooks), emit.
- Hydration by default.
- Return thunk from onremove with `remove` function.
- Return actions from app().
@jbrodriguez jbrodriguez mentioned this pull request Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants