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

Wrap handlers #59

Merged
merged 6 commits into from
Sep 7, 2016
Merged

Wrap handlers #59

merged 6 commits into from
Sep 7, 2016

Conversation

yoshuawuyts
Copy link
Owner

@yoshuawuyts yoshuawuyts commented Jul 22, 2016

Adds the functionality described in choojs/choo#145. Builds on #58. Pretttttty excited about this ✨

  • implementation
  • docs
  • tests

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+0.5%) to 95.152% when pulling 7e17e82 on wrap-handlers into b381a97 on master.

@myobie
Copy link

myobie commented Jul 29, 2016

I am very excited about this. I already created an effectWrapper function that returns a wrapped function so I can use another promise based storage api more easily in the effects, but I have to use it repeatedly in my model.

An example I have is:

export function wrapEffect (cb) {
  return async (data, state, send, done) => {
    const newSend = (action, data = null) => {
      return new Promise((resolve, reject) => {
        send(name, data, (err, res) => {
          if (err) {
            done(err) // communicate error to choo
            reject(err) // throw
          } else {
            resolve(res) // move forward
          }
        })
      })
    }
    await cb(data, state, newSend)
    done() // auto-done when everything is done
  }
}

which allowed me to write this in the end:

app.model({
  effects: {
    reloadMailsCount: wrapEffect(async (data, state, send) => {
      const count = await storage.getMailsCount()
      await send('receiveMailsCount', count)
    })
  }
})

Is this the type of thing you are hoping to enable?

@yoshuawuyts
Copy link
Owner Author

yoshuawuyts commented Jul 29, 2016

@myobie Hah yeah, pretty much! I'm a strong opponent of Promises myself, but I want people that like them to be able to use them as they please (as with other paradigms).

A little note: for your implementation to be correct you'd probably have to wrap it in a try ... catch so any errors that bubble up can be caught for the send(err) call. Also probably cool to allow it to return a value synchronously which would be passed into send(null, value)

@myobie
Copy link

myobie commented Jul 29, 2016

@yoshuawuyts great idea, I will do that. And yes, promises + try/catch do sometimes make my head hurt.

I'm not entirely sure what send(null, value) would do, so I will do some research there.

@yoshuawuyts
Copy link
Owner Author

yoshuawuyts commented Jul 29, 2016

@myobie so when calling send(name, data, callback(err, res)) the receiving method can trigger the callback by calling done(err, res). It's basically delegation of control, and once it's done it either returns an error or data (where data can be null / undefined). This essentially creates a CSP style system. This is not explicitly mentioned anywhere though (yay for docs) but figured I'd mention it here so that you can at least have a good time implementing a package in this fashion (':

@coveralls
Copy link

coveralls commented Sep 7, 2016

Coverage Status

Coverage decreased (-0.5%) to 97.972% when pulling e6c28e0 on wrap-handlers into 7756160 on master.

@coveralls
Copy link

coveralls commented Sep 7, 2016

Coverage Status

Coverage increased (+0.2%) to 98.586% when pulling e72f758 on wrap-handlers into 7756160 on master.

@yoshuawuyts yoshuawuyts merged commit bf29aec into master Sep 7, 2016
@yoshuawuyts yoshuawuyts deleted the wrap-handlers branch September 7, 2016 15:37
yoshuawuyts added a commit to choojs/choo that referenced this pull request Sep 7, 2016
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.

None yet

3 participants