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(build): Have prepare task signal async completion #6356

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

cpcallen
Copy link
Contributor

@cpcallen cpcallen commented Aug 18, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

"Did you forget to signal async completion?" error from gulp when running the prepare task (as invoked by npm run prepare or npm install).

Proposed Changes

Add a done parameter and pass the provided callback to buildJavaScriptAndDeps. This also allows a simplification of the the don't-run case by simply calling done() and then returning.

Behaviour Before Change

When not run by a GitHub action, npm run prepare and npm install generate the aforementioned error.

Behaviour After Change

prepare script successfully signals async completion, avoiding error.

Reason for Changes

PR #6244 made a change to have the npm run prepare script (automatically invoked by npm install after package installation) not bother running the buildJavaScriptAndDeps gulp task when run from a GitHub action (since our build action will do npm test straight afterwards, which runs this task).

Unfortunately, due my misunderstanding of how gulp tasks work the "fix" was not correct, and somehow I overlooked the error while testing locally (and of course it was not generated in CI because the other branch of the conditional was taken).

Test Coverage

  • Tested locally to verify that buildJavaScriptAndDeps is run and no error generated.
  • Tested in CI to verify that buildJavaScriptAndDeps is not run and no error generated.

No manual testing changes envisioned.

PR google#6244 made a change to have the npm run prepare script
(automatically invoked by npm install after package installation)
not bother running the buildJavaScriptAndDeps gulp task when run
from a GitHub action (since our build action will do npm test
straight afterwards, which runs this task already).

Unfortunately, due my misunderstanding of how gulp tasks work
the revised version generated a "Did you forget to signal async
completion?" error from gulp.

Fix this by adding a done parameter and passing the provided
callback to buildJavaScriptAndDeps.  This also allows a
simplification of the the don't-run case by simply calling
done and then returning.
@rachel-fenichel rachel-fenichel merged commit 079699b into google:develop Aug 22, 2022
@cpcallen cpcallen deleted the fix-prepare branch September 21, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants