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

add parameter for indicating an array of actions not to buffer #15

Merged
merged 3 commits into from
Aug 14, 2017

Conversation

brownbl1
Copy link
Contributor

I think this should address issue #14. I also added a very similar test to show what my changes would do.

Also, would this really constitute a breaking change if we only are adding an additional parameter? The first test passed without any modifications so in that sense it wouldn't effect current consumers.

Just let me know what you think!

index.js Outdated
@@ -2,38 +2,39 @@ var BUFFERED_ACTION_RETURN = 'redux-action-buffer: buffered action'

var setImmediate = typeof global !== 'undefined' && typeof global.setImmediate !== 'undefined' ? global.setImmediate : setTimeout

module.exports = function bufferActions (breaker, cb) {
module.exports = function bufferActions(breaker, cb, passthrough) {
Copy link
Owner

Choose a reason for hiding this comment

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

how about this api instead:
bufferActions(options, cb)
where options is an object with { breaker, passthrough }.

This way we can keep backwards compat via mapping the arguments if a string is provided in the first argument. Also keeps callback as final arg which is idiomatic.

@rt2zz
Copy link
Owner

rt2zz commented Aug 14, 2017

i.e.

module.exports = function bufferActions(options, cb) {
  var active = true
  var queue = []
  var passthrough = options && options.passthrough || []
   
  var breaker = typeof options === 'object'
    ? options.breaker
    : options

  var breakerType = typeof breaker
  //...
}

Kind of messy but it should work. That or we can just make a breaking change and release a new major version.

@brownbl1
Copy link
Contributor Author

Yeah, I like that better too. I'll clean it up in the morning.

@brownbl1
Copy link
Contributor Author

Changes made, and I also updated the readme to reflect the new argument definition. Feel free to tweak it. Tests still pass so I think we're good?

@rt2zz rt2zz merged commit b2d9d55 into rt2zz:master Aug 14, 2017
@brownbl1
Copy link
Contributor Author

Awesome! Thanks. Also, I realized I forgot to bump the minor version in package.json. Could you do that and push a new package to npm when you get a chance?

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.

2 participants