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

fix series() to run in sequence #4

Merged
merged 3 commits into from
Feb 10, 2017
Merged

fix series() to run in sequence #4

merged 3 commits into from
Feb 10, 2017

Conversation

haltcase
Copy link
Contributor

@haltcase haltcase commented Feb 2, 2017

Fixes #3

I'm not able to build or run tests since I'm on Windows (separate discussion - consider using cross-platform npm scripts eventually?) so you'll want to confirm. I did run a quick test script of my own though, which was this:

import { series } from './src'

let i = 0
const delayed = ms => {
  return new Promise(resolve =>
    setTimeout(() => (console.log(++i), resolve(i)), ms)
  )
}

series([
  () => delayed(1000),
  () => delayed(1000),
  () => delayed(1000),
  () => delayed(1000),
  () => delayed(1000)
]).then(res => {
  console.log('done', res)
})

You'll see the numbers 1-5 logged with about a second between each and then 'done' [1, 2, 3, 4, 5].

@haltcase haltcase changed the title fix series() to run in sequence [WIP] fix series() to run in sequence Feb 2, 2017
@haltcase
Copy link
Contributor Author

haltcase commented Feb 2, 2017

After seeing the Travis test results I remarked this a WIP because I totally fixated on running the functions in series, and not so much as an ordered version of Promise.all. So this doesn't return the array of function results like it should.

Fixed it. This basically uses the solution mentioned by @TuckerWhitehouse but with a couple improvements:

  1. drops resolve() usage entirely since it's actually not needed for series()
  2. remove return await and just return the reduce() result - this is something I'd actually recommend across the whole package since using it adds an extra tick.

@haltcase haltcase changed the title [WIP] fix series() to run in sequence fix series() to run in sequence Feb 2, 2017
@haltcase
Copy link
Contributor Author

@developit, you may not have had time to review but I'm just checking in - let me know if you have any thoughts on this.

@developit
Copy link
Owner

Apologies, I've been busy this week but I reviewed and it looks good.

Copy link
Owner

@developit developit left a comment

Choose a reason for hiding this comment

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

LGTM.

@developit developit merged commit 4cdc581 into developit:master Feb 10, 2017
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.

Series is executing in parallel
2 participants