build: move bootstrap to postinstall hook by bajtos · Pull Request #1264 · loopbackio/loopback-next · GitHub

bajtos · 2018-04-17T09:05:14Z

Rework setup/build scripts to run lerna bootstrap as part of npm install.

The new setup simplifies our development workflow when changing branches. Instead of having to remember and type multiple npm scripts, one can run a single command:

$ npm install-test

Or even shorter:

$ npm it

Under the hood, a new "postinstall" script is calling "lerna bootstrap" to install dependencies in individual packages after the top-level monorepo dependencies were installed first.

Checklist

npm test passes on your machine
(n/a) New tests added or existing tests modified to cover all changes
Code conforms with the style guide
(n/a) API Documentation in code was updated
Documentation in /docs/site was updated
(n/a) Affected artifact templates in packages/cli were updated
(n/a) Affected example projects in examples/* were updated

Run "npm lint:fix" to apply the new formatting introduced by the recent prettier version.

Rework setup/build scripts to run "lerna bootstrap" as part of "npm install". The new setup simplifies our development workflow when changing branches. Instead of having to remember and type multiple npm scripts, one can run a single command: npm install-test # abbreviated as: npm it Under the hood, a new "postinstall" script is calling "lerna bootstrap" to install dependencies in individual packages.

bajtos · 2018-04-17T10:51:55Z

A downside of my proposal: The commit linting phase run by Travis CI is going to unnecessary run lerna boostrap as part of the automatically executed npm install step. I have few ideas what to try, I'll push them as fixup commits.

bajtos · 2018-04-17T10:42:10Z

  depth: 300

-before_script:
-  - npm run bootstrap


Travis automatically runs npm install before running any before_script entries. There is no need to run npm install again!

bajtos · 2018-04-17T10:42:51Z


  async function deleteSandbox() {
-    if (!await pathExists(path)) return;
+    if (!(await pathExists(path))) return;


This change is needed to fix the CI after a recent prettier version changed the formatting of await statements. It comes in a standalone commit.

Please also upgrade to "prettier": "^1.12.1" so that the new code won't fail with existing checkout which still uses prettier@1.11.x.

Rework setup/build scripts to run "lerna bootstrap" as part of "npm install". The new setup simplifies our development workflow when changing branches. Instead of having to remember and type multiple npm scripts, one can run a single command: npm install-test # abbreviated as: npm it Under the hood, a new "postinstall" script is calling "lerna bootstrap" to install dependencies in individual packages.

bajtos · 2018-04-17T11:03:56Z

I configured the linting step to run npm install --ignore-scripts, this brought down the time to complete the linting phase from 2m:26s to 57 seconds. See 373c1ba

bajtos · 2018-04-17T11:04:31Z

-  - npm run bootstrap
-
-script:
-  - npm test


npm test is the default build script for Node.js projects, no need to explicitly specify it in our Travis config.

virkt25 · 2018-04-17T13:46:32Z

Need to fix one of the commit messages!

raymondfeng · 2018-04-17T18:02:13Z

For the record, there is an overhead (a few secs) to run npm install + lerna bootstrap over lerna bootstrap. Since I can still run npm run postinstall or npx lerna bootstrap, I'm good with the changes.

raymondfeng

Please investigate the misbehaving npm it.

raymondfeng · 2018-04-17T22:24:29Z

    "clean:lerna": "lerna clean",
    "build": "node bin/run-lerna run build",
-    "build:full": "npm run clean:lerna && npm run bootstrap && npm test",
+    "build:full": "npm run clean:lerna && npm install-test",


I'm seeing very strange behavior of npm install-test. It triggers two runs of install and test. The package-lock.json is also generated even with .npmrc that disables package-lock.

See npm/npm#20358

Interesting. Let's hold on my pull request until npm/npm#20358 is resolved.

Rework setup/build scripts to run "lerna bootstrap" as part of "npm install". The new setup simplifies our development workflow when changing branches. Instead of having to remember and type multiple npm scripts, one can run a single command: npm install-test # abbreviated as: npm it Under the hood, a new "postinstall" script is calling "lerna bootstrap" to install dependencies in individual packages.

b-admike

LGTM once npm/npm#20358 is resolved

raymondfeng · 2018-04-18T15:21:48Z

@bajtos If you want to land this PR sooner, use npm install && npm test to replace npm install-test.

bajtos · 2018-04-18T15:24:11Z

I personally use npm it a lot. If this patch is going to break it, then I'd rather wait until a fixed version of npm is available.

raymondfeng · 2018-04-20T15:16:56Z

Please note npm/npm#20358 has been merged but it will take some time to be released. Even we get a new release of npm with the fix, are we going to mandate the minimum version of npm for loopback-next?

bajtos · 2018-04-23T09:52:26Z

Please note npm/npm#20358 has been merged but it will take some time to be released. Even we get a new release of npm with the fix, are we going to mandate the minimum version of npm for loopback-next?

Good point. I am going to rework this pull request to use npm install && npm test then.

Rework setup/build scripts to run "lerna bootstrap" as part of "npm install". The new setup simplifies our development workflow when changing branches. Instead of having to remember and type multiple npm scripts, one can run a single command: npm install-test # abbreviated as: npm it Under the hood, a new "postinstall" script is calling "lerna bootstrap" to install dependencies in individual packages.

bajtos added this to the April 2018 milestone Apr 17, 2018

bajtos self-assigned this Apr 17, 2018

bajtos requested review from b-admike, raymondfeng and virkt25 April 17, 2018 09:05

bajtos added the needs-priority label Apr 17, 2018

bajtos force-pushed the build/bootstrap-at-install branch from ec5e1df to 2bd3135 Compare April 17, 2018 10:41

bajtos added a commit that referenced this pull request Apr 17, 2018

build: update prettier formatting (#1264)

db08271

Run "npm lint:fix" to apply the new formatting introduced by the recent prettier version.

bajtos commented Apr 17, 2018

View reviewed changes

bajtos force-pushed the build/bootstrap-at-install branch from 6cf9855 to 373c1ba Compare April 17, 2018 11:01

bajtos commented Apr 17, 2018

View reviewed changes

virkt25 approved these changes Apr 17, 2018

View reviewed changes

shimks approved these changes Apr 17, 2018

View reviewed changes

jannyHou approved these changes Apr 17, 2018

View reviewed changes

raymondfeng approved these changes Apr 17, 2018

View reviewed changes

raymondfeng requested changes Apr 17, 2018

View reviewed changes

bajtos force-pushed the build/bootstrap-at-install branch from 8bcef81 to 2f8e522 Compare April 18, 2018 13:04

b-admike approved these changes Apr 18, 2018

View reviewed changes

bajtos force-pushed the build/bootstrap-at-install branch from 2f8e522 to 5bb6ce7 Compare April 23, 2018 10:03

bajtos force-pushed the build/bootstrap-at-install branch from 5bb6ce7 to 367d587 Compare April 23, 2018 10:04

b-admike approved these changes Apr 23, 2018

View reviewed changes

raymondfeng approved these changes Apr 23, 2018

View reviewed changes

bajtos force-pushed the build/bootstrap-at-install branch from 367d587 to 415910a Compare April 24, 2018 08:54

bajtos merged commit c926031 into master Apr 24, 2018

bajtos deleted the build/bootstrap-at-install branch April 24, 2018 10:20

bajtos removed the needs-priority label Apr 24, 2018

Conversation

bajtos commented Apr 17, 2018

Checklist

Uh oh!

bajtos commented Apr 17, 2018

Uh oh!

bajtos Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

bajtos Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

raymondfeng Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 17, 2018

Uh oh!

bajtos Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

virkt25 commented Apr 17, 2018

Uh oh!

raymondfeng commented Apr 17, 2018

Uh oh!

raymondfeng left a comment

Choose a reason for hiding this comment

Uh oh!

raymondfeng Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

bajtos Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Apr 18, 2018

Uh oh!

bajtos commented Apr 18, 2018

Uh oh!

raymondfeng commented Apr 20, 2018

Uh oh!

bajtos commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

raymondfeng Apr 17, 2018 •

edited

Loading