feat: Add build scripts as a separate package #682

raymondfeng · 2017-10-26T22:49:30Z

Description

The PR adds build package to the monorepo. It will be published as @loopback/build. The module exposes a set of build related scripts.

Other modules will use @loopback/build as a dev dependency and use the cli commands for the build.

Related issues

connect to <link_to_referenced_issue>

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

bajtos

Nice start!

I am not sure how much value there is in sharing our build scripts that are specific to monorepo structure, but then I don't mind that too much.

In addition to my comments below, please take a look at monorepo-level tslint.* config files, they are still around after your changes:

Most importantly, make sure that the tslint extension for VSCode is picking up the right lint config.

Similarly, there are tsconfig.* files at root level that are used by the VSCode TS intellisense, they must be updated to refer to the shared config from the new build package.

bajtos · 2017-10-27T12:16:00Z

packages/authentication/package.json

-    "LoopBack",
-    "Authentication"
-  ],
+  "keywords": ["LoopBack", "Authentication"],


FWIW, we are not (or should not be) applying prettier formatting to package.json, because npm will reformat the file back to JSON.stringify format whenever we change it, typically via npm install. Could you please revert these white-space only changes?

I am not sure if package.json is listed in .prettierignore, feel free to add it there if it isn't. Here is what we did in loopback4-extension-starter (the trick works well with VS Code prettier plugin):

https://github.com/strongloop/loopback4-extension-starter/blob/3a491d40e7d58ad177484de459050ba127215af7/.prettierignore#L4

*.json

bajtos · 2017-10-27T12:17:07Z

packages/build/.prettierrc

@@ -0,0 +1,6 @@
+{


I think we should be using top-level .prettierrc from the monorepo, not per-package configs. Please remove this file.

This is intentional as I want to make sure the scripts are formatted with es5.

I see, makes sense 👍

bajtos · 2017-10-27T12:18:07Z

packages/build/CODEOWNERS

@@ -0,0 +1,7 @@
+# Lines starting with '#' are comments.


Ditto, CODEOWNERS file does not make sense inside a package. Please move this entry to top-level CODEOWNERS file at monorepo's root level and use a path specifier to limit the codeowners to this package only.

bajtos · 2017-10-27T12:21:58Z

packages/build/config/nyc.common.json

@@ -0,0 +1,6 @@
+{
+  "include": ["packages/example-codehub/src/**", "packages/*/dist*/*"],
+  "exclude": ["packages/core/*/promisify.*"],


This configuration is specific to loopback-next (include packages/example-codehub, exclude promisify.* from core), it does not belong to a shared/common config.

bajtos · 2017-10-27T12:24:41Z

packages/context/package.json

+    "unit":
+      "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js'",
+    "verify":
+      "npm pack && tar xf loopback-context*.tgz && tree package && npm run clean"


Same comment applies here - please revert whitespace changes, npm is going to revert them anyways.

Please do the same in all package.json files modified by this PR.

bajtos · 2017-10-27T12:31:48Z

The CI build is failing on all platforms (Travis, AppVeyor + Node.js 6.x, 8.x), please check.

bajtos · 2017-10-27T12:32:45Z

packages/build/README.md

+    "build:dist": "lb-tsc es2017",
+    "build:dist6": "lb-tsc es2015",
+    "build:apidocs": "lb-apidocs",
+    "tslint": "lb-tslint",


This is cool if the project uses the same tslint config as we do. However, if the project wants to customize their tslint settings, how can they do that?

bajtos · 2017-10-27T12:33:18Z

packages/build/README.md

+    "build:apidocs": "lb-apidocs",
+    "tslint": "lb-tslint",
+    "tslint:fix": "npm run tslint -- --fix",
+    "prettier:cli": "lb-prettier \"**/*.ts\"",


This is cool if the project uses the same prettier config as we do. However, if the project wants to customize their settings, how can they do that? Could you please explain that in README?

raymondfeng · 2017-10-27T17:07:33Z

I am not sure how much value there is in sharing our build scripts that are specific to monorepo structure, but then I don't mind that too much.

The @loopback/build is not only for the monorepo. I was motivated to create this package for other repos such as loopback4-extension-grpc and loopback4-extension-starter. Once the PR is merged, I'll create other PRs to switch to @loopback/build for these modules.

raymondfeng · 2017-10-27T17:07:49Z

@bajtos PTAL.

b-admike

Although this PR is a bit big to review, I don't really have any concerns. I like how the package is composed of many (mainly independent) build scripts so that people can pick and choose what they want out of it.

b-admike · 2017-10-27T17:17:52Z

tsconfig.json

-    "packages/*/node_modules/**",
-    "**/*.d.ts"
-  ]
+  "extends": "./packages/build/config/tsconfig.common.json",


Do we need the following line here as well?

"$schema": "http://json.schemastore.org/tsconfig"

b-admike · 2017-10-27T17:51:44Z

packages/build/test/integration/utils.test.js

+const path = require('path');
+const fs = require('fs');
+
+const utils = require('../../bin/utils');


We don't really use utils in this test case. Can we test its exported functions here as well as the scripts in /bin? You can test them in separate files or describe blocks, but I leave the organization up to you.

Good catch. Fixed.

virkt25 · 2017-10-27T19:20:24Z

packages/build/bin/lint-commits.sh

@@ -0,0 +1,45 @@
+#!/bin/bash


This file doesn't need to be moved as we don't expose it in build and loopback-next still seems to be using the one from /bin.

raymondfeng · 2017-10-28T20:23:01Z

@bajtos PTAL

kjdelisle

I'm mostly happy about the scripts no longer containing tons of ../../../../../

bajtos

I am afraid I don't have enough time to review these changes in all details and I don't want to block progress. The changes LGTM at high level now.

My only concern is about mono-repo's top-level tsconfig.build.json which seems to mostly duplicate the content in packages/build/config/tslint.build.json.

bajtos · 2017-10-31T13:50:03Z

packages/build/.prettierrc

@@ -0,0 +1,6 @@
+{


I see, makes sense 👍

raymondfeng requested a review from bajtos as a code owner October 26, 2017 22:49

raymondfeng added needs-priority and removed needs-priority labels Oct 26, 2017

raymondfeng force-pushed the add-buildlab branch from 9e902f9 to 934bb73 Compare October 26, 2017 23:01

raymondfeng requested a review from kjdelisle as a code owner October 27, 2017 05:31

raymondfeng force-pushed the add-buildlab branch from 488e2df to 363d398 Compare October 27, 2017 06:05

raymondfeng changed the title ~~[WIP] feat: Add build scripts as a separate package~~ feat: Add build scripts as a separate package Oct 27, 2017

raymondfeng force-pushed the add-buildlab branch from 363d398 to 3a2a542 Compare October 27, 2017 06:08

bajtos suggested changes Oct 27, 2017

View reviewed changes

bajtos reviewed Oct 27, 2017

View reviewed changes

raymondfeng force-pushed the add-buildlab branch 3 times, most recently from a7e2066 to bfec8ae Compare October 27, 2017 17:03

raymondfeng force-pushed the add-buildlab branch from bfec8ae to 5d6bfec Compare October 27, 2017 17:08

b-admike approved these changes Oct 27, 2017

View reviewed changes

b-admike reviewed Oct 27, 2017

View reviewed changes

raymondfeng force-pushed the add-buildlab branch 3 times, most recently from b42e96f to 08fdb9d Compare October 27, 2017 18:55

virkt25 reviewed Oct 27, 2017

View reviewed changes

raymondfeng force-pushed the add-buildlab branch 5 times, most recently from 185a4ab to c6f7599 Compare October 27, 2017 20:26

raymondfeng force-pushed the add-buildlab branch 2 times, most recently from 78ccf5f to a96a680 Compare October 27, 2017 21:19

raymondfeng force-pushed the add-buildlab branch 2 times, most recently from 6fc98e1 to 9d36527 Compare October 30, 2017 20:40

raymondfeng mentioned this pull request Oct 30, 2017

build: Switch to @loopback/build strongloop-archive/loopback4-extension-starter#16

Merged

Raymond Feng and others added 2 commits October 30, 2017 14:53

feat: Add build scripts as a separate package

d7cf7b2

build: Switch to @loopback/build

2efb764

raymondfeng force-pushed the add-buildlab branch from 9d36527 to 2efb764 Compare October 30, 2017 21:53

kjdelisle approved these changes Oct 31, 2017

View reviewed changes

bajtos approved these changes Oct 31, 2017

View reviewed changes

packages/build/.prettierrc

@@ -0,0 +1,6 @@

{

Copy link

Member

bajtos Oct 31, 2017

Choose a reason for hiding this comment

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

I see, makes sense 👍

raymondfeng merged commit f3d3a26 into master Oct 31, 2017

raymondfeng removed the review label Oct 31, 2017

kjdelisle deleted the add-buildlab branch October 31, 2017 14:45

bajtos mentioned this pull request Dec 15, 2017

Shared tslint and tsconfig #174

Closed

bajtos mentioned this pull request Feb 8, 2018

docs: add CONTRIBUTING, DEVELOPING and VSCODE #977

Merged

6 tasks

bajtos mentioned this pull request Feb 16, 2018

Fix tsc setup to report project-relative paths in error messages #1010

Closed

2 tasks

feat: Add build scripts as a separate package #682

feat: Add build scripts as a separate package #682

Uh oh!

Conversation

raymondfeng commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Oct 27, 2017

Uh oh!

raymondfeng commented Oct 27, 2017

Uh oh!

b-admike left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Oct 28, 2017

Uh oh!

kjdelisle left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

raymondfeng commented Oct 26, 2017 •

edited

Loading

b-admike left a comment •

edited

Loading