Skip to content

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Nov 11, 2016

This PR adds the concept of effects to Microcosm, a way to apply side-effects in a safe way. Whenever an action moves into a new state, it asks the history object to reconcile. After reconciliation, I've added an extra step for handling side effects. This should support a few use cases:

  1. Side-effects in presenters that are the result of actions happening
  2. Secondary data fetching
const repo = new Microcosm()

const Notifications = {
  notify (repo, params) {
    alert(params.name + ' was updated!')
  }

  register() {
    return {
      [createUser] : this.notify
    }
  }
}

repo.addEffect(Notifications)

repo.push(createUser, { name: 'Billy' })

These can be added to Presenters too, and localized to a particular fork. So side-effects can be managed more strategically than a simple register method:

class MyPresenter extends Presenter {
    setup (repo) {
        repo.addEffect(Effect, { options })
    }
}

function format (string) {
/*eslint-disable no-unused-vars*/
const [ _, action, state = 'done' ] = `${ string }`.match(/(\w*)\_\d+[\_\w]*/) || []
/*eslint-enable no-unused-vars*/
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, nice. Didn't realize you could do that inline. Sad that we can't make a global exception for this pattern 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack! This file should get removed. Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beyond that though, I want to say there's a thing you can do with no-unused-vars that ignores variables that start with _.

@greypants
Copy link
Contributor

greypants commented Nov 11, 2016

I like it. I need to sit with the term Effect for a bit, but I think this is super helpful. Minor concerns/things to think about: Domains, Presenters, and Effects are all feel super similar in that they all have register methods and callbacks. A newbie might wonder why they can't just use domains for this purpose, since they look the same and work similarly. Maybe just an issue of documentation though.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 72b7fc8 on effects into d4da2cc on master.

@greypants
Copy link
Contributor

greypants commented Nov 11, 2016

This probably screws up a bunch of naming internals, but I almost think this would be nice:

class MyPresenter extends Presenter {
    setup (repo) {
        repo.register(Effects, { options })
    }
}

Just an idea. Not married to it. Just thinking that repo.addEffect could be clearer or more descriptive. Or not!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 62a113f on effects into d62f888 on master.

@nhunzaker
Copy link
Contributor Author

@greypants I hear you on the similarity. There's something there that I haven't figured out yet. I'm okay with it being a little confusing for the sake of consistency, but I agree that the documentation needs to be clearer.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 16c7388 on effects into 36121a7 on master.

@nhunzaker nhunzaker deleted the effects branch November 30, 2016 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants