Drop support for Node.js 6 #948

b-admike · 2018-02-01T13:57:57Z

Cherry pick some commits from #945 (see #945 (review)). 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

bajtos

Few minor comments, the patch looks good in general 👏

bajtos · 2018-02-01T14:13:13Z

.travis.yml

  - "8"

+os:
+  - linux
+  - osx


Can we leave osx support out of this patch please?

Yeah I was in the process of removing it :)

bajtos · 2018-02-01T14:13:30Z

.travis.yml

-      node_js:
-        - "8"
+      os:
+        - linux


bajtos · 2018-02-01T14:16:39Z

packages/rest/src/parser.ts

-const parseJsonBody: (req: ServerRequest) => Promise<MaybeBody> = promisify(
-  jsonBody,
-);
+const jsonBodyAsync = promisify(require('body/json'));


Please preserve the name parseJsonBody, method/function names should start with a verb.

Also please verify that the compiler is correctly inferring the type of promisify(require('body/json')) and does not fall back to any. If it does not, then you need to preserve some of the type definitions above.

bajtos

Oh, hang on. I just realized that we need to remove build:dist6 scripts from our package.json files. There is no point in transpiling for ES2015 (Node.js 6.x) when we no longer support it.

bajtos · 2018-02-01T15:06:50Z

Oh, hang on. I just realized that we need to remove build:dist6 scripts from our package.json files. There is no point in transpiling for ES2015 (Node.js 6.x) when we no longer support it.

On the second thought - when we remove build:dist6, then we must change package-level index.js files too, because we can no longer forward to dist6, at which point we may be too close to #945?

I am no longer sure what's the right approach here. Does anybody else have any opinion?

The only important thing that remains is to ensure the commit message landing these changes contains BREAKING CHANGE footer, so that our release notes explicitly mention this change. See https://conventionalcommits.org/

For example:

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.

I am no longer sure it's necessary to remove dist6 as part of this PR.

virkt25 · 2018-02-01T15:51:35Z

Need to add a Test stage for Travis because right now only thing running is Commit Lint because not having a os field or multiple versions of node means there is no longer a default stage Travis generates. Right now the only thing Travis is doing is commit message linting.

b-admike · 2018-02-01T16:18:03Z

@bajtos re: #948 (comment), perhaps we can add 4a62b0e to this PR instead of the other?

Co-authored-by: Taranveer Virk <[email protected]>

kjdelisle

LGTM,
as long as you fix the Travis stuff.

kjdelisle · 2018-02-01T21:29:34Z

It does suck that we're losing almost 1% of code coverage because of this change, but that's more indicative of a problem we already had than one we're creating.

b-admike · 2018-02-01T21:46:10Z

Yeah I'm trying to figure out why there is a sudden drop of coverage :(.

b-admike · 2018-02-01T21:47:42Z

@virkt25 @kjdelisle Travis should be good now that we landed #949.

Dropping node 6 support from this PR and util.promisify

bajtos · 2018-02-02T09:33:52Z

t does suck that we're losing almost 1% of code coverage because of this change, but that's more indicative of a problem we already had than one we're creating.

I checked Coveralls report. The only relevant change in coverage numbers I can see are for rest/src/parser.ts.

I suspect our coverage numbers by me skewed by the fact that we are reporting coverage for each Node.js platform we are building. Because the code emitted by tsc is different, especially for Node.js 6.x with no support for async/await, I suspect the coverage transformed via sourcemaps could provide slightly different data on each Node.js version.

I personally don't worry about the exact coverage number, especially when it's well over 90%. For me, coverage data is just a tool helping us to spot parts of code that are not sufficiently covered by tests.

The Travis setup seems to work wonderfully 👍 Let's get this landed! 🎉

richardpringle · 2018-02-02T21:29:31Z

package.json

@@ -6,7 +6,7 @@
  },
  "version": "4.0.0-alpha.1",
  "engines": {
-    "node": ">=6"
+    "node": ">=8"


Dropping node 6 support from this PR and util.promisify

b-admike requested review from bajtos, jannyHou, kjdelisle, raymondfeng, shimks and virkt25 as code owners February 1, 2018 13:57

b-admike changed the title ~~Chore/drop node6~~ Drop support for Node.js 6 Feb 1, 2018

b-admike assigned b-admike and virkt25 Feb 1, 2018

b-admike removed the request for review from virkt25 February 1, 2018 14:09

b-admike mentioned this pull request Feb 1, 2018

ci: add osx to build matrix #949

Merged

6 tasks

bajtos approved these changes Feb 1, 2018

View reviewed changes

b-admike force-pushed the chore/drop-node6 branch from 17ed9de to 0c9b09a Compare February 1, 2018 14:17

bajtos previously requested changes Feb 1, 2018

View reviewed changes

biniam and others added 3 commits February 1, 2018 15:10

ci: drop node6 from travis and appveyor configs

8c4ccc2

build: change node version to 8 or greater

5471694

build: replace/remove node6 promisify helpers

df338ad

Co-authored-by: Taranveer Virk <[email protected]>

b-admike force-pushed the chore/drop-node6 branch from 0c9b09a to df338ad Compare February 1, 2018 20:10

fixup! apply feedback

f90abf2

b-admike force-pushed the chore/drop-node6 branch from 49a4324 to f90abf2 Compare February 1, 2018 20:56

kjdelisle approved these changes Feb 1, 2018

View reviewed changes

jannyHou approved these changes Feb 1, 2018

View reviewed changes

virkt25 added a commit that referenced this pull request Feb 2, 2018

fixup! drop node 6 in anticipation of #948

3419003

Dropping node 6 support from this PR and util.promisify

virkt25 mentioned this pull request Feb 2, 2018

feat: @loopback/boot #858

Merged

6 tasks

bajtos approved these changes Feb 2, 2018

View reviewed changes

bajtos merged commit 05de86c into master Feb 2, 2018

bajtos deleted the chore/drop-node6 branch February 2, 2018 09:34

richardpringle reviewed Feb 2, 2018

View reviewed changes

virkt25 added a commit that referenced this pull request Feb 5, 2018

fixup! drop node 6 in anticipation of #948

ebd0eda

Dropping node 6 support from this PR and util.promisify

virkt25 added a commit that referenced this pull request Feb 6, 2018

fixup! drop node 6 in anticipation of #948

0bb1917

Dropping node 6 support from this PR and util.promisify

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for Node.js 6 #948

Drop support for Node.js 6 #948

b-admike commented Feb 1, 2018 •

edited

Loading

bajtos left a comment

bajtos Feb 1, 2018

b-admike Feb 1, 2018

bajtos Feb 1, 2018

bajtos Feb 1, 2018

bajtos left a comment

bajtos commented Feb 1, 2018

virkt25 commented Feb 1, 2018

b-admike commented Feb 1, 2018

kjdelisle left a comment •

edited

Loading

kjdelisle commented Feb 1, 2018

b-admike commented Feb 1, 2018 •

edited

Loading

b-admike commented Feb 1, 2018

bajtos commented Feb 2, 2018

richardpringle Feb 2, 2018

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

Drop support for Node.js 6 #948

Drop support for Node.js 6 #948

Conversation

b-admike commented Feb 1, 2018 • edited Loading

Checklist

bajtos left a comment

Choose a reason for hiding this comment

bajtos Feb 1, 2018

Choose a reason for hiding this comment

b-admike Feb 1, 2018

Choose a reason for hiding this comment

bajtos Feb 1, 2018

Choose a reason for hiding this comment

bajtos Feb 1, 2018

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Feb 1, 2018

virkt25 commented Feb 1, 2018

b-admike commented Feb 1, 2018

kjdelisle left a comment • edited Loading

Choose a reason for hiding this comment

kjdelisle commented Feb 1, 2018

b-admike commented Feb 1, 2018 • edited Loading

b-admike commented Feb 1, 2018

bajtos commented Feb 2, 2018

richardpringle Feb 2, 2018

Choose a reason for hiding this comment

b-admike commented Feb 1, 2018 •

edited

Loading

kjdelisle left a comment •

edited

Loading

b-admike commented Feb 1, 2018 •

edited

Loading