Skip to content

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