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

Promises and tests #1

Closed
wants to merge 1 commit into from
Closed

Conversation

MajorBreakfast
Copy link

  • Promises: Broccoli uses promises pretty extensively. Consequently they're the perfect match for quick-temp
  • Tests: For the three functions to ensure they work as advertised

Note: This PR is a breaking change. There is no way not make it non-breaking because going async is inherently a breaking change.

@joliss
Copy link
Owner

joliss commented Mar 12, 2014

Thanks for the PR! Hm, what's the motivation for using promises? Is there a use case that is not covered by the original synchronous implementation?

@MajorBreakfast
Copy link
Author

Sync has problems. Isaac also recommends using async https://github.com/isaacs/rimraf

Promises vs callbacks: Promises are slightly less performant but nicer to work with. (We could add callbacks, too, if the need be.)

@joliss
Copy link
Owner

joliss commented Mar 13, 2014

Sync has problems.

I think when you're seeing the async code work better, it's not because of any fundamental reason why async is more robust, but rather because async enables rimraf to do icky setTimeout-based workarounds that hide the problem rather than fixing it. I don't think that enabling these workarounds is sufficient reason to make quick-temp async. (Also see https://github.com/joliss/broccoli/issues/39#issuecomment-37486171.)

Isaac also recommends using async

The rimraf README is rather cryptic, but I assume that this recommendation is because the sync version doesn't have said workarounds. If you like you can ping Isaac about this and find out more. For now, I'm not willing to merge this PR, but if there turns out to be a deeper problem with the sync code, I'd be happy to revisit this.

@joliss joliss closed this Mar 13, 2014
@MajorBreakfast
Copy link
Author

Hmm. Okay, I release it under https://github.com/MajorBreakfast/slick-temp :)

While I agree that it's bad that node's sync file functions throw more errors I suspect the reason for that is that they're not the way to go and as such haven't gotten enough developer attention. Blocking the main thread just isn't right. While at the moment with the rise of SSDs we've gotten pretty fast file systems I suspect that gap will widen again. For CPUs price and speed play a role. For memory price, speed and capacity play a role. I say that async code is more future proof. Using all that sync code also goes a bit against node's nature in general.

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