Skip to content
This repository was archived by the owner on Aug 11, 2022. It is now read-only.

Conversation

markpeterfejes
Copy link
Contributor

Hey!

This small PR fixes #17707 by executing the git add commands sequentially instead of concurrently, which sometimes causes the aforementioned error.

I'm not really sure that this is the prettiest way to implement this, change suggestions are very welcome!

Thank you!

@markpeterfejes markpeterfejes requested a review from a team as a code owner July 12, 2017 11:29
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

needs a small mostly-stylistic change

lib/version.js Outdated
], options)
}
}).nodeify(cb)
}

function stagePackageFiles (localData, options) {
return new BB(function (resolve, reject) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need a promise constructor here. Just:

return addLocalFile('package.json', options, false).then(() => {
  if (localData.hasShrinkwrap) {
    return addLocalFile('npm-shrinkwrap.json', options, false)
  } else if (localData.hasPackageLock) {
    return addLocalFile('package-lock.json', options, false)
  }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing this right now! Is there a possibility that this could be merged to the 5.3.0 relese today?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably! I'll take a look in the AM and see if this patch is ready to merge. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much!

@markpeterfejes
Copy link
Contributor Author

Should be ready for release :)

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

changes look good, everything seems to work. gj, and thanks for the contrib! 🎉

@zkat zkat changed the base branch from latest to release-next July 13, 2017 21:08
@zkat zkat merged this pull request into npm:release-next Jul 13, 2017
zkat pushed a commit that referenced this pull request Jul 13, 2017
* Sequential promises instead of concurrent

* Fixed style errors

* Removed the promise constructor

PR-URL: #17742
Credit: @markpeterfejes
Reviewed-By: @zkat
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants