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

Execute action functors by wrapping them in setTimeout. #23

Merged
merged 4 commits into from
Jul 30, 2014

Conversation

dashed
Copy link
Contributor

@dashed dashed commented Jul 30, 2014

Closes #22.

@spoike spoike added this to the 0.1.4 milestone Jul 30, 2014
@dashed
Copy link
Contributor Author

dashed commented Jul 30, 2014

I put setTimeout as a dependency injection via utils.nextTick.

@spoike
Copy link
Member

spoike commented Jul 30, 2014

I'd rather avoid adding dependencies to the project (much like we did earlier with removing lodash as dependency). Even though nextTick is a small library, it seems unneccesary for the scope of our project.

I believe it's better in this case, for the sake of smaller footprint for browsers, to let the user choose to inject nextTick themselves through set nextTick and have setTimeout be the default implementation. Also add documentation on the README on how to inject nextTick.

@spoike
Copy link
Member

spoike commented Jul 30, 2014

Alternatively we could try to configure browserify to ignore the nextTick when building, and have Reflux use setTimeout default implementation if nextTick dependency is missing. Not sure how to do that though, I thought I saw some project doing this.

The goal is that the npm package should contain the nextTick package, while the bower component will have it optional.

@dashed
Copy link
Contributor Author

dashed commented Jul 30, 2014

I don't follow. The next-tick module is a polyfill for browsers, why would you want browserify to ignore it?


I amended the commit.

@spoike
Copy link
Member

spoike commented Jul 30, 2014

Because the user might be using the polyfill in their own build. No need to have it twice, once in their own project and second from our project. It's just unnecessary overhead.

@dashed
Copy link
Contributor Author

dashed commented Jul 30, 2014

Fine by me. Just leave it [the module] out.

@spoike
Copy link
Member

spoike commented Jul 30, 2014

All right!

@spoike
Copy link
Member

spoike commented Jul 30, 2014

I'll review the tests later tonight before merging.

@dashed
Copy link
Contributor Author

dashed commented Jul 30, 2014

@dashed
Copy link
Contributor Author

dashed commented Jul 30, 2014

Turns out asap, as a name, already means something different than what setTimeout does:
http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony


The best option is a polyfilled setImmediate: https://github.com/YuzuJS/setImmediate

Reason: http://www.nczonline.net/blog/2013/07/09/the-case-for-setimmediate/

setTimeout is probably a decent shim, at least in chrome which takes care of its optimizations: https://code.google.com/p/chromium/issues/detail?id=146172#c10

Updated README on this.

spoike added a commit that referenced this pull request Jul 30, 2014
Execute action functors by wrapping them in setTimeout.
@spoike spoike merged commit 4eb7896 into reflux:master Jul 30, 2014
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.

Wrap action functions in setTimeout?
2 participants