Simplify build infrastructure #945

b-admike · 2018-01-31T21:01:03Z

Related to #611

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

shimks

Couple of questions

shimks · 2018-01-31T21:05:14Z

package.json

@@ -37,7 +37,7 @@
    "build": "lerna run build --parallel --loglevel=silent",
    "build:current": "lerna run build:current --parallel --loglevel=silent",


Is there still need for this?

nope, good catch!

shimks · 2018-01-31T21:07:33Z

packages/cli/generators/project/templates/package.json

@@ -6,7 +6,7 @@
    "loopback-<%= project.projectType -%>",
    "loopback"
  ],
-  "main": "index.js",
+  "main": "dist/index.js",
  "engines": {
    "node": ">=8"
  },


remove dist6 from files (way down; github won't let me comment on unchanged lines)

shimks · 2018-01-31T21:12:44Z

packages/core/package.json

@@ -38,7 +35,6 @@
    "index.js",
    "index.d.ts",


Do we still need this (and index.js)?

See #945 (comment)

raymondfeng · 2018-01-31T21:51:37Z

packages/build/bin/utils.js

@@ -12,11 +12,12 @@ const spawn = require('cross-spawn');
 const debug = require('debug')('loopback:build');

 /**
- * Get the Node.js compilation target - es2017 or es2015
+ * Get the Node.js compilation target - es2017


IMO, we should keep es2015 support for @loopback/build as it can be used to build TypeScript projects independent of LoopBack 4.

+1

Also as I mentioned above, I'd like us to add es2018 target soon (around April this year, when Node.js 10.0.0 is released).

V8 6.4 already supports all ES2018 features according to this tweet

Node's master branch contains V8 6.4 AFAICT, see deps/v8

(I do realize this contradicts the checklist in #945, which I created back in October last year.)

raymondfeng · 2018-01-31T21:53:12Z

packages/cli/generators/project/templates/package.json

-    "index.d.ts",
-    "dist",
-    "dist6"
+    "dist/index.js",


Is it redundant with dist?

raymondfeng · 2018-01-31T21:59:26Z

packages/core/package.json

  },
  "devDependencies": {
    "@loopback/build": "^4.0.0-alpha.12",
    "@loopback/testlab": "^4.0.0-alpha.22"
  },
  "files": [
    "README.md",
-    "index.js",
-    "index.d.ts",
+    "dist/index.js",


Please note we have the following files under dist. We should include all of them except test. The top level index.* files are useful for source map.

index.d.ts index.js index.js.map src test

raymondfeng

Please address my comments.

jannyHou · 2018-01-31T22:24:07Z

.travis.yml

@@ -24,8 +27,8 @@ jobs:
      before_script: skip
      script: /bin/bash ./bin/lint-commits.sh
      after_success: skip
-      node_js:
-        - "8"
+      os:


any reason why osx is not in the list?

Commit message linting only needs to happen once (on any platform) so for this stage we explicitly specify linux. Not specifying any will trigger commit message linting on both platforms which is redundant.

bajtos

Besides my comments above, I am proposing to split this pull request into two smaller incremental changes:

Drop support for Node.js in engines field, CI config, remove promisify pollyfils. I think this part is pretty non-controversial.
Simplify project infrastructure (package-level index.js files, @loopback/build, etc.) I think these changes require more careful consideration and discussion.

bajtos · 2018-02-01T08:37:22Z

.travis.yml

  - "8"

+os:
+  - linux
+  - osx


Please don't make two changes in one pull request. Adding osx to our build matrix should be done in a standalone pull request.

The historic reason for this addition is that Travis was only running commit linter instead of our build, since there was no longer a matrix with 1e5bc76. TV and I added it to address that issue. I agree that is a change on its own, so I can create a PR for it.

Wow, I did not realize a single Node.js target would break the matrix build. Thanks for explaining this to me. Your other PR looks good to me. Another option we can consider is to add Node.js 9 to the list of platforms we are testing.

bajtos · 2018-02-01T08:40:44Z

packages/authentication/index.js

-// License text available at https://opensource.org/licenses/MIT
-
-const nodeMajorVersion = +process.versions.node.split('.')[0];
-module.exports = nodeMajorVersion >= 7 ? require('./dist') : require('./dist6');


With Node.js 10.0.0 coming in few months, I think we should start planning how to transpile to es2018 target. Since transpiling for both Node.js 8.x and 10.x will require these index files around, I think we should preserve them.

https://twitter.com/jasnell/status/958765477089632257

PSA: Mark your calendars, the expected release date for Node.js 10.0.0 is on or around April 24th. Expect the first As-Close-To-A-Release-Candidate-As-We-Can-Get-It to hit in early April.

Thoughts?

(I do realize this contradicts the checklist in #945, which I created back in October last year.)

After thinking about this a bit more: we should leave out transpilation target es2018 out of scope of this pull request.

If we need to keep index.js file around because of #945 (comment), then I am ok with simplifying it to a simple redirect to dist. If it turns out TS+VSCode works well without this hand-written index.js around, then I am ok with deleting it as is proposed here.

Actually yeah we were discussing about es2018 before making this change. I think it might be better to keep them as they do no harm, and if used in the future, we don't have to remake them.

bajtos · 2018-02-01T08:46:39Z

packages/authentication/package.json

  },
+  "main": "dist/index.js",


❗️ This is critical ❗️

I think this will break monorepo-wide refactorings in VSCode. Before your change, the TypeScript server was seeing packages/authentication/index.ts and ignoring any .js/.d.ts files produced by our build. With your change in place, the TypeScript server will see packages/authentication/dist/index.js (and index.d.ts), which will hide the original TypeScript sources.

Here are things that will stop working if my assessment is correct:

"Go to definition" across packages. How to test: find a place where we are calling @inject in authentication package, press F12 to go to the definition of inject. Right now, VSCode should jump to the .ts file in src. With your changes in place, I believe VSCode will jump to a .d.ts file in dist

Refactorings like "rename" will not change all places using the renamed entity. Two different scenarios to verify: rename at the place where the entity is defined, rename at the place where the entity is used. (You can e.g. rename inject to test.)

Please verify whether the three scenarios mentioned above work with your change in place. If they do, then my understanding of TypeScript integration in VSCode is no longer correct and you can ignore my comment.

Good catch! ~~The first scenario works, but the other two do not. Would ignoring those files in their new locations fix this issue? (will try and check back)~~

EDIT: All of those scenarios work fine with these changes in place, but yeah I'm still in favour of preserving our target selector index.js files.

bajtos · 2018-02-01T08:49:31Z

packages/build/bin/utils.js

@@ -12,11 +12,12 @@ const spawn = require('cross-spawn');
 const debug = require('debug')('loopback:build');

 /**
- * Get the Node.js compilation target - es2017 or es2015
+ * Get the Node.js compilation target - es2017


+1

Also as I mentioned above, I'd like us to add es2018 target soon (around April this year, when Node.js 10.0.0 is released).

V8 6.4 already supports all ES2018 features according to this tweet

Node's master branch contains V8 6.4 AFAICT, see deps/v8

bajtos · 2018-02-01T08:50:33Z

packages/cli/generators/app/templates/index.ts

+// Add any aditional (re)exports to src/index.ts instead.
+export * from './src';
+
+const application = (module.exports = require('./src'));


My two comment above advocating for preservation of package-level index.js file apply to this place too.

bajtos · 2018-02-01T09:47:57Z

Closes #611

@b-admike This phrase is telling GitHub to close #611 automatically after this pull request is landed. Are you sure all other tasks in #611 will be done by that time? In my experience, it's usually better to close user stories manually and avoid using automatic closing via "closes" or "fixes".

b-admike · 2018-02-01T14:29:13Z

@bajtos Sorry, should not have used closes phrase. There are still outstanding checkboxes in that task that I need to make PRs for in their respective repos. I've put related instead.

bajtos

LGTM with one suggestion.

bajtos · 2018-02-05T12:34:30Z

packages/authentication/index.js

@@ -3,5 +3,6 @@
 // This file is licensed under the MIT License.
 // License text available at https://opensource.org/licenses/MIT

+/* eslint-disable no-unused-vars*/
 const nodeMajorVersion = +process.versions.node.split('.')[0];


I think we should remove nodeMajorVersion if it's not used (instead of adding eslit-disable comment).

The same comment applies to all other places below where you added eslint-disable.

(BTW I believe we don't have eslint configured in our monorepo, do you have eslint configured in your IDE independently of our project setup?)

Okay, will do. Yeah I have eslint, but realized we use tslint after, and I have that change in the other PRs :).

BREAKING CHANGE: Support for Node.js version lower than 8.0 has been dropped. Please upgrade to the latest Node.js 8.x LTS version. Co-Authored-by: Taranveer Virk <[email protected]>

b-admike self-assigned this Jan 31, 2018

b-admike requested review from bajtos, jannyHou, kjdelisle, raymondfeng, shimks and virkt25 as code owners January 31, 2018 21:01

b-admike added the needs-priority label Jan 31, 2018

b-admike removed the request for review from virkt25 January 31, 2018 21:03

b-admike assigned virkt25 Jan 31, 2018

shimks reviewed Jan 31, 2018

View reviewed changes

b-admike force-pushed the build/drop-node6 branch from 76b03a4 to 56560e4 Compare January 31, 2018 21:23

raymondfeng reviewed Jan 31, 2018

View reviewed changes

raymondfeng requested changes Jan 31, 2018

View reviewed changes

jannyHou reviewed Jan 31, 2018

View reviewed changes

bajtos suggested changes Feb 1, 2018

View reviewed changes

b-admike mentioned this pull request Feb 1, 2018

Drop support for Node.js 6 #948

Merged

6 tasks

b-admike changed the title ~~Drop support for Node.js 6~~ Simplify build infrastructure Feb 1, 2018

b-admike force-pushed the build/drop-node6 branch from 56560e4 to 18b6265 Compare February 1, 2018 14:10

b-admike force-pushed the build/drop-node6 branch 5 times, most recently from 6955730 to 9868d64 Compare February 2, 2018 16:27

bajtos approved these changes Feb 5, 2018

View reviewed changes

b-admike force-pushed the build/drop-node6 branch from ce1796f to a8f0b20 Compare February 5, 2018 18:17

raymondfeng approved these changes Feb 5, 2018

View reviewed changes

b-admike force-pushed the build/drop-node6 branch from a8f0b20 to 9f9c9a5 Compare February 6, 2018 02:37

build: drop dist6 related targets

2b4f822

BREAKING CHANGE: Support for Node.js version lower than 8.0 has been dropped. Please upgrade to the latest Node.js 8.x LTS version. Co-Authored-by: Taranveer Virk <[email protected]>

b-admike force-pushed the build/drop-node6 branch from 9f9c9a5 to 2b4f822 Compare February 6, 2018 03:43

b-admike merged commit a2368ce into master Feb 6, 2018

b-admike deleted the build/drop-node6 branch February 6, 2018 05:48

b-admike removed the needs-priority label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify build infrastructure #945

Simplify build infrastructure #945

b-admike commented Jan 31, 2018 •

edited

Loading

shimks left a comment

shimks Jan 31, 2018

b-admike Jan 31, 2018

shimks Jan 31, 2018

shimks Jan 31, 2018

b-admike Jan 31, 2018

bajtos Feb 5, 2018

raymondfeng Jan 31, 2018

bajtos Feb 1, 2018

bajtos Feb 1, 2018

raymondfeng Jan 31, 2018

raymondfeng Jan 31, 2018

raymondfeng left a comment

jannyHou Jan 31, 2018

virkt25 Feb 1, 2018

bajtos left a comment

bajtos Feb 1, 2018

b-admike Feb 1, 2018

bajtos Feb 1, 2018

bajtos Feb 1, 2018

bajtos Feb 1, 2018

bajtos Feb 1, 2018

b-admike Feb 1, 2018

bajtos Feb 1, 2018 •

edited

Loading

b-admike Feb 1, 2018 •

edited

Loading

bajtos Feb 1, 2018

bajtos Feb 1, 2018

bajtos commented Feb 1, 2018

b-admike commented Feb 1, 2018 •

edited

Loading

bajtos left a comment

bajtos Feb 5, 2018

b-admike Feb 5, 2018

		@@ -37,7 +37,7 @@
		"build": "lerna run build --parallel --loglevel=silent",
		"build:current": "lerna run build:current --parallel --loglevel=silent",

                 - "8"
+              os:
+                - linux
+                - osx

Simplify build infrastructure #945

Simplify build infrastructure #945

Conversation

b-admike commented Jan 31, 2018 • edited Loading

Checklist

shimks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

b-admike Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 1, 2018

b-admike commented Feb 1, 2018 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike commented Jan 31, 2018 •

edited

Loading

bajtos Feb 1, 2018 •

edited

Loading

b-admike Feb 1, 2018 •

edited

Loading

b-admike commented Feb 1, 2018 •

edited

Loading