Skip to content

Drop Node.js 6 support #80

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.

Sign up for GitHub

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

Jump to bottom
Merged
b-admike merged 1 commit into from
Feb 9, 2018
Merged

Drop Node.js 6 support #80

b-admike merged 1 commit into from
Feb 9, 2018

Conversation

@b-admike
Copy link
Member

@b-admike b-admike commented Feb 8, 2018

LoopBack4 is dropping support for Node 6 because it is no longer LTS and doesn't support newer features. It's best for this example repo to align with it.

BREAKING CHANGE: Support for Node.js version lower than 8.0 has been dropped. Please upgrade to Node 8 or higher.

Connect to loopbackio/loopback-next#611

bajtos
bajtos approved these changes Feb 9, 2018
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please remove these files:

  • services/account/package-lock.json
  • services/customer/package-lock.json
  • services/facade/package-lock.json
  • services/todo-legacy/package-lock.json
  • services/transaction/package-lock.json

Other than that, I don't see any obvious problems.

@bajtos
Copy link
Member

bajtos commented Feb 9, 2018

Oh, please check why [cis-jenkins] x64 && linux && nvm,8 failed and consider configuring Travis CI for this repo while you are making these changes.

@bajtos
Copy link
Member

bajtos commented Feb 9, 2018

Please remove these files:

LOL, I got confused when reading the diff. Your pull request is already removing those package-lock files, that's great!

So the only blocker is the fact that the only CI we have configured (cis-jenkins) is failing.

kjdelisle
kjdelisle previously requested changes Feb 9, 2018
View reviewed changes
Copy link
Contributor

@kjdelisle kjdelisle left a comment

Choose a reason for hiding this comment

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

Some minor fixes required, but almost ready to land.

services/account/test/unit/test.data.json Outdated
{
"ids": {
"Account": 17
"Account": 23
Copy link
Contributor

@kjdelisle kjdelisle Feb 9, 2018

Choose a reason for hiding this comment

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

This value shouldn't have changed (technically, this test needs rewriting because this value shouldn't be perpetually increasing in commits!).

Copy link
Contributor

@kjdelisle kjdelisle Feb 9, 2018

Choose a reason for hiding this comment

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

To be clear; don't rewrite the test logic as a part of this commit. Just revert this change.

@kjdelisle
Copy link
Contributor

kjdelisle commented Feb 9, 2018

@b-admike

Tests are failing due to this issue:

17:07:12 /home/jenkins/workspace/nb/loopback-next-example~master/13482bc3/node_modules/ts-node/src/index.ts:307
17:07:12         throw new TSError(formatDiagnostics(diagnosticList, cwd, ts, lineOffset))
17:07:12               ^
17:07:12 TSError: ⨯ Unable to compile TypeScript
17:07:12 services/todo-legacy/test/controller/todo-controller.test.ts (15,20): Expected 1 arguments, but got 0. (2554)
17:07:12     at getOutput (/home/jenkins/workspace/nb/loopback-next-example~master/13482bc3/node_modules/ts-node/src/index.ts:307:15)
17:07:12     at /home/jenkins/workspace/nb/loopback-next-example~master/13482bc3/node_modules/ts-node/src/index.ts:336:16
17:07:12     at Object.compile (/home/jenkins/workspace/nb/loopback-next-example~master/13482bc3/node_modules/ts-node/src/index.ts:498:11)
17:07:12     at Module.m._compile (/home/jenkins/workspace/nb/loopback-next-example~master/13482bc3/node_modules/ts-node/src/index.ts:392:43)
17:07:12     at Module._extensions..js (module.js:654:10)
17:07:12     at Object.require.extensions.(anonymous function) [as .ts] (/home/jenkins/workspace/nb/loopback-next-example~master/13482bc3/node_modules/ts-node/src/index.ts:395:12)

Are you seeing this compilation failure locally?

@b-admike
Copy link
Member Author

b-admike commented Feb 9, 2018

@kjdelisle @bajtos it was passing fine on local :(, but will fix those up. on a side note, @shimks and I were trying to figure out where the dist folders were being generated after running the compiler, since they were nowhere to be found.

jannyHou
jannyHou reviewed Feb 9, 2018
View reviewed changes
services/todo-legacy/controllers/todo-controller.ts
@@ -1,15 +1,17 @@
import { api, Application, inject } from '@loopback/core';
import { Application, inject } from '@loopback/core';
import { api } from '@loopback/rest';
Copy link

@jannyHou jannyHou Feb 9, 2018

Choose a reason for hiding this comment

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

@b-admike we export @api from @loopback/rest only for backward compatibility, it is now defined and exported from @loopback/openapi-v2

Not a necessary to change them to @loopback/openapi-v2 in this PR since that package will be renamed in v3 in the near future.

Copy link
Member Author

@b-admike b-admike Feb 9, 2018

Choose a reason for hiding this comment

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

@jannyHou sounds good. Are you ok with making those changes in #78?

Copy link

@jannyHou jannyHou Feb 9, 2018

Choose a reason for hiding this comment

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

Sure~ 👍

Drop Node.js 6 support
a6de58c 
LoopBack4 is dropping support for Node 6 because
it is no longer LTS and doesn't support newer
features. It's best for this example repository
to align with it.

BREAKING CHANGE: Support for Node.js version lower
than 8.0 has been dropped. Please upgrade to Node
8 or higher.
@b-admike
Copy link
Member Author

b-admike commented Feb 9, 2018

@slnode test please

@b-admike b-admike dismissed kjdelisle’s stale review February 9, 2018 22:13

Applied feedback :-)

@b-admike b-admike merged commit 1acbe4c into master Feb 9, 2018
@b-admike b-admike deleted the update-nodever branch February 9, 2018 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jannyHou jannyHou jannyHou approved these changes

@dhmlau dhmlau Awaiting requested review from dhmlau dhmlau is a code owner

@shimks shimks Awaiting requested review from shimks

@virkt25 virkt25 Awaiting requested review from virkt25

+2 more reviewers

@bajtos bajtos bajtos approved these changes

@kjdelisle kjdelisle kjdelisle left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@b-admike @bajtos @kjdelisle @jannyHou