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

Execute hooks in series as per pre-v10 releases #812

Closed
wants to merge 1 commit into from
Closed

Execute hooks in series as per pre-v10 releases #812

wants to merge 1 commit into from

Conversation

benlancaster
Copy link

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Since v10, the functions provided to afterCopy, afterExtract and afterPrune no longer execute in series. In some cases, this behaviour is desirable since one hook may effect changes that another hook assumes have already happened. I couldn't see anything about this change in the release notes, so this PR restores that behaviour whilst still using Promises internally.

If this change is welcome, could someone please give me some pointers getting the unit tests passing? I'm having a hard time, either seeing intermittent failures on the sequential ordering assertions (presumably because the packagerTest builds two packages), or the following:

  platform=all test (one arch) (afterPrune hook)

  Rejected promise returned by test. Reason:

  Error {
    cmd: 'npm prune --production',
    code: 1,
    killed: false,
    signal: null,
    message: `Command failed: npm prune --production␊
    npm WARN @4.99.101 No description␊
    npm WARN @4.99.101 No repository field.␊
    npm WARN @4.99.101 No license field.␊
    ␊
    npm ERR! May not delete: /private/var/folders/nz/s1gw0mtj4jq8jqc7w2p090tr0000gn/T/75089d8c4e4d0acbb0bcbe90f07b7de5/electron-packager/linux-ia32/packagerTest-linux-ia32/resources/app/node_modules/.bin␊
    ␊
    npm ERR! A complete log of this run can be found in:␊
    npm ERR!     /Users/username/.npm/_logs/2018-03-16T16_58_32_665Z-debug.log␊
    `,
  }

@welcome
Copy link

welcome bot commented Mar 16, 2018

Thanks for opening a pull request!

Here are some highlighted action items that will help get it across the finish line, from the
pull request guidelines:

  • Follow the JavaScript coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes in NEWS.md and other docs.
  • Include tests when adding/changing behavior.

Development and triage is community-driven, so please be patient and we will get back to you as soon as we can.

@@ -149,7 +149,11 @@ module.exports = {
return Promise.resolve()
}

return Promise.all(hooks.map(hookFn => pify(hookFn).apply(this, args)))
const promisified = hooks.map(hookFn => pify(hookFn).bind(this, ...args))
Copy link
Author

Choose a reason for hiding this comment

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

What's the right way to do this without the spread operator?

Copy link
Author

Choose a reason for hiding this comment

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

In fact it looks like Node 4 will be EOL at the end of April, so I guess this could stay in if this gets accepted and released after that

@malept
Copy link
Member

malept commented Mar 20, 2018

I appreciate the effort you made in this PR, however, I'm unlikely to revert the feature change to the hooks. As a counterproposal, I've written #814, which I hope addresses your concerns.

@benlancaster
Copy link
Author

Yeah that's great, thank you.

@benlancaster benlancaster deleted the sequential-hooks branch April 26, 2018 10:09
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