Skip to content

fix series() to run in sequence#4

Merged
developit merged 3 commits intodevelopit:masterfrom
haltcase:patch-1
Feb 10, 2017
Merged

fix series() to run in sequence#4
developit merged 3 commits intodevelopit:masterfrom
haltcase:patch-1

Conversation

@haltcase
Copy link
Contributor

@haltcase haltcase commented Feb 2, 2017 •
edited
Loading

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 •
edited
Loading

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

haltcase commented Feb 10, 2017

@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

developit commented Feb 10, 2017

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

developit
developit approved these changes Feb 10, 2017
View reviewed changes
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

Reviewers

@developit developit developit approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Series is executing in parallel

2 participants

@haltcase @developit