Update examples to work with latest loopback versions #78

thinusn · 2017-11-05T12:52:41Z

Description

Updated the examples (accounts, customer, transactions & facade) to work with the latest versions of @loopback/*.

The example now runs and builds with

    "@loopback/boot": "^0.4.0",
    "@loopback/core": "^0.3.0",
    "@loopback/repository": "^0.3.0",
    "@loopback/rest": "^0.4.0",
    "@types/cors": "^2.8.3",
    "@loopback/build": "^0.3.2",
    "@loopback/testlab": "^0.4.0"

I added tests for customer and transaction and updated the existing account tests. I also changed the example slightly to make the facade work. All tests for the examples are passing.

Related issues

connect to example doesn't run #76

Checklist

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

slnode · 2017-11-05T12:52:43Z

Can one of the admins verify this patch?

b-admike

@thinusn Thank you for your contribution! I've reviewed until f245f5b and left some comments. Will review the rest of your commits in another iteration.

b-admike · 2017-11-06T13:13:45Z

services/account/test/unit/test.ts


-  accCtrl.repository = new AccountRepository(


do we have a use for test.data.json after removing it here?

b-admike · 2017-11-06T13:20:51Z

services/account/test/unit/test.data.json

@@ -10,4 +10,4 @@
      "CHK99999999": "{\"id\":\"CHK99999999\",\"customerNumber\":\"0002444422\",\"balance\":100.89,\"branch\":\"Foster City\",\"type\":\"Checking\",\"avgBalance\":100.93,\"minimumBalance\":10}"
    }
  }
-}
+}


newline here

b-admike · 2017-11-06T13:23:11Z

services/account/index.ts

+        );
+        super(options);
+
+        const dataSource: juggler.DataSource = new DataSourceConstructor('local-fs', {


I think it would make more sense to define datasource elsewhere in its own file (for e.g. /datasources/mem.datasource.ts), and only bind it in this file.

b-admike · 2017-11-06T13:23:43Z

services/account/controllers/AccountController.ts

-   async deleteAccount(where) {
-    return await this.repository.deleteAccount(JSON.parse(where));
-  }
+    async updateAccount(where = '', data) {


why are we setting where to an empty string here?

b-admike · 2017-11-06T13:24:41Z

services/account/controllers/AccountController.ts

+        return await this.repository.updateAll(data, JSON.parse(where));
+    }
+
+    async deleteAccount(where = '{}') {


ditto; why are we setting where filter to an empty object?

b-admike · 2017-11-06T13:25:42Z

services/account/repositories/account/models/account/account.ts

@@ -0,0 +1,4 @@
+import {Entity, model} from '@loopback/repository';
+
+@model(require('./accoun-definition.json'))


./accoun-definition.json -> ``./account-definition.json`

b-admike · 2017-11-06T13:25:50Z

services/account/repositories/account/models/account/account.ts

+import {Entity, model} from '@loopback/repository';
+
+@model(require('./accoun-definition.json'))
+export class Account extends Entity {}


newline here

bajtos · 2018-02-05T10:33:27Z

@thinusn @b-admike hello, what's the status of this pull request? I think we will need even more changes now that several more alpha versions of LoopBack 4 have been released.

b-admike · 2018-02-06T06:07:21Z

@bajtos I did a first round of review, but did not do any more. @thinusn has applied the feedback, and I'll work with him to get the changes up-to-date with our latest changes in lb-next.

dhmlau · 2018-02-18T04:27:02Z

@thinusn, there have been quite a lot of changes since your PR has been submitted. Could you please rebase your PR, and also sign the CLA? Thanks!

b-admike · 2018-03-07T20:42:19Z

@thinusn I have rebased and added 3 commits which aim to fix the account service and can serve you as a guide to fix up the rest of the services to be caught up with the current version of loopback-next. @strongloop/sq-lb-apex can I get a review on them (EDIT: 3 last commits) so that @thinusn and I are on the right track?

kjdelisle

Looking good, just a few changes:

needs to restore the original formatting of some files
remove package-lock files
fix missing newlines (GitHub's diff will show you where using the 🚫 emoji/symbol)

kjdelisle · 2018-03-07T20:47:00Z

services/account-without-juggler/controllers/AccountController.ts

+import { api } from '@loopback/rest';
+import { def } from './AccountController.api';
+import { AccountRepository } from '../repositories/account';
+import { inject } from '@loopback/context';


Our style guide in the loopback monorepo requires that there be no added spaces after the brackets and imports.
import {inject} from '@loopback/context';

I know that this is a different repository, but the existing code is also following that style, so we should leave it as-is. If you've got something auto-formatting it, then you should reconfigure it for that style.

yeah we now have linting in this repo thanks to #81, so this should be fixed by running npm run lint:fix. cc @thinusn

kjdelisle · 2018-03-07T20:50:48Z

services/account-without-juggler/repositories/account/models/Account.ts

@@ -2,11 +2,10 @@ import {Entity, model, ModelDefinition} from '@loopback/repository';

 @model(require('./account/model-definition'))


Is this still required now that the @model decorator automatically generates metadata based on the constructor definition?

good point. we can probably get rid of it!

kjdelisle · 2018-03-07T20:52:54Z

services/account-without-juggler/test/unit/account-controller.ts

@@ -1,3 +1,4 @@
+// test/unit/account-controller.ts


Unnecessary comment.

kjdelisle · 2018-03-07T21:05:36Z

services/account/test/unit/test.data.json

@@ -1,6 +1,6 @@
 {
  "ids": {
-    "Account": 23
+    "Account": 5


We should probably:

make a one-time setup to generate this data file if it doesn't exist

ignore it in .gitignore so that it can stop showing up in changes

(this is optional for this review)

kjdelisle · 2018-03-07T21:11:35Z

services/account/tsconfig.json

  ],
  "exclude": [
+    "node_modules/**",
+    "packages/*/node_modules/**",
    "**/*.d.ts"
  ]
 }


missing newline (it wasn't your fault, but since we're here... :) )

kjdelisle · 2018-03-07T21:12:29Z

services/customer/package-lock.json

@@ -0,0 +1,1893 @@
+{


No package-lock.json files, please.

jannyHou

@thinusn Thank you for the contribution.

We recently upgraded to OpenAPI 3.0.0 and I left comments for the specs need to be upgraded.

But before upgrading anything, I want to confirm with team @strongloop/loopback-next is this example repo aimed on the API first approach? Or we can refactor the code to generate apis by decorators?

jannyHou · 2018-03-08T19:05:05Z

services/account/src/controllers/account.controller.api.ts

@@ -24,6 +24,26 @@ export const def = {
        },
      },
    },
+    '/accounts/{id}': {


The OpenAPI specifications are upgraded to 3.0.0, please update the path spec accordingly, see document https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#operationObject

The corresponding 3.0.0 spec would be:

get: { 'x-operation-name': 'getAccount', parameters: [ { name: 'id', in: 'path', description: 'Model id', required: true, // please note the 3.0.0 organizes properties like `type`, `format` are under `schema`. schema: { type: 'string', format: 'JSON', } }, ], responses: { 200: { // please note 3.0.0 supports multiple content-types, // change it to `*/*` or any working content-type '*/*': { // And 2.0 definitions are organized to components/schemas schema: '#/components/schemas/Account', }, }, }, }, },

My code may not work out-of-box, please double check it.

jannyHou · 2018-03-08T19:20:07Z

services/account/src/models/account.definition.ts

@@ -0,0 +1,34 @@
+export const accountDefinition = {


I think we can decorate the model by

@model Class Account { @property('id') id:string, // ...etc }

If any controller method has argument with type Account, then the OpenAPI schema for Account will be auto-generated.

jannyHou · 2018-03-08T19:22:36Z

services/customer/controllers/CustomerController.api.ts

          {
            name: 'filter',
            in: 'query',
-            description:
-              'The criteria used to narrow down the number of customers returned.',
+            description: 'The criteria used to narrow down the number of customer returned.',


The specifications here also need to be updated as OpenAPI 3.0.0.

jannyHou · 2018-03-08T19:24:39Z

services/facade/repositories/account/swagger.json

@@ -7,14 +7,14 @@
  "host": "localhost:3001",
  "basePath": "/",
  "paths": {
-    "/accounts": {
+    "/accounts/{id}": {


This should be updated as openapi.json.

jannyHou · 2018-03-08T19:27:52Z

services/facade/repositories/customer/swagger.json

-            },
-            "deprecated":false
-         }  
+  "swagger": "2.0",


This should be updated as OpenAPI 3.0.0 spec as well.

jannyHou · 2018-03-08T19:28:34Z

services/transaction/controllers/TransactionController.api.ts

-    },
+    }
+  },
+  definitions: {


same ^ OpenAPI 3.0.0

jannyHou · 2018-03-08T19:28:53Z

services/facade/repositories/transaction/swagger.json

-            },
-            "deprecated":false
-         }  
+  "swagger": "2.0",


Same ^ OpenAPI 3.0.0

thinusn · 2018-03-10T10:52:49Z

services/account/src/application.ts

+/* tslint:disable:no-unused-variable */
+import {
+  DataSourceConstructor,
+  juggler,


Instead of doing this, would it not be better to do

import * as lbRepository from '@loopback/repository'; import * as lbBoot from '@loopback/boot';

and then the class would be defined as follows

export class AccountMicroservice extends lbBoot.BootMixin(lbRepository.RepositoryMixin(RestApplication)) {

That way we don't have to deal with the unused-variable ts error, and the IDE's auto formatting removing these unused imports?

While that removes the unused-variable tslint error, it's good practice to import only what we need from those modules as it makes it clear what we use from them. This is the approach we take in loopback-next and it'd be good to follow it here.

b-admike · 2018-03-15T14:37:19Z

@thinusn mocha version has been upgraded in #90, so you also need to rebase after your changes. If you need help with any of the feedback, please let me know.

b-admike · 2018-03-20T22:49:35Z

hey @thinusn are you still interested to work on this PR?

thinusn · 2018-03-21T16:23:42Z

@b-admike jup, I just need the time to sit down actually do it
I managed to do a few things today: linting, updated the tests, made some changes based on feedback above, got the accounts service working again and some other updates (types etc.). Will try and get the others services up to date.

b-admike · 2018-03-22T13:47:51Z

package.json

@@ -39,9 +39,9 @@
    "bluebird": "^3.5.0",
    "mocha": "^5.0.3",
    "ts-node": "^3.1.0",
-    "typescript": "^2.6.1"
+    "typescript": "^2.4.1"


Is there a reason for downgrading typescript?

nope good catch will make it the same version as https://github.com/strongloop/loopback-next/blob/master/packages/build/package.json

b-admike · 2018-03-22T20:17:23Z

@thinusn Excellent work. Thank you for committing in small increments and following our commit message guidelines! I'm going to review your latest commits, but so far so good (I've reviewed a bunch and haven't found anything that stands out).

thinusn · 2018-03-23T10:33:39Z

Account, Customer Transaction and Facade is now working again and up to date with latest lb versions . I made some changes to how the app starts all the services and general improvements / updates throughout.

thinusn · 2018-03-23T10:37:06Z

services/facade/src/repositories/account/account.repository.ts

+import {dataSource} from './swagger.datasource';
+/* tslint:disable no-any */
+export class AccountRepository {
+  model: any;


@b-admike How do we want to get the types into the Facade? I did not want to import them from eg. '../../../account/bla/bla' as we then have a direct coupling. For now I just made the types any, perhaps this example should also be a lerna project in the future, that way the facade can just import the types.

+1 for setting up Lerna-based monorepo in this repository. The sooner we can make that transition, the better.

thinusn · 2018-03-23T10:37:52Z

services/facade/src/repositories/account/account.repository.api.ts

@@ -0,0 +1,76 @@
+export const accountDefinition = {
+  swagger: '2.0',


The loopback-connector-swagger packages does not support openapi 3.0 :(

I'm not too caught up on the purpose of facade, but is it necessary for the package to be using loopback-connector-swagger @strongloop/lb-next-dev ?

shimks

Thanks for fixing up what was in need of some dire upgrade! Here's a couple of high-level comments that may help you finish this PR quicker and easier:

going over the README again to see if it's still accurate after the update
users need to give explicit permission to execute shell scripts, we should do this for them in our scripts
we should leverage lerna to resolve dependencies between folders if needed
use automatic openapi spec generating decorators (like @get, @param, etc.)
add properties to existing models and use the model class as types
complete switching repositories over to extending from DefaultCrudRepository
make sure to include dist folders in .gitignore

I think the scope of this task only includes the first two points (README and shell script execution privilege) and the rest would be more appropriate for a refactor in a separate PR.

Please don't hesitate to ping us if you need help in getting this PR landed :)

shimks · 2018-03-27T17:03:57Z

services/account-without-juggler/index.ts

+    options = Object.assign(
+      {},
+      {
+        components: [RestComponent],


We don't support binding components via the constructor anymore.
Please bind the component through app.component(RestComponent) instead.
Alternatively, the application could extend from RestApplication instead, which already has RestComponent and RestServer registered.

shimks · 2018-03-27T17:13:22Z

services/account/src/controllers/account.controller.api.ts

+  },
+  components: {
+    schemas: {
+      Account: accountDefinition,


Please create a spec for Account here as well

shimks · 2018-03-27T17:27:14Z

services/customer/src/application.ts

+  }
+
+  setupDataSources() {
+    this.bind('dataSources.memory').to(dataSource);


This isn't important, but we now have a sugar for this in RepositoryMixin as this.dataSource(dataSource) :)

shimks · 2018-03-27T17:29:56Z

services/customer/src/controllers/customer.controller.api.ts

+  },
+  components: {
+    schemas: {
+      Customer: customerDefinition,


customerDefinition isn't a valid SchemaObject according to petstore.editor. It can't have key name in it.

shimks · 2018-03-27T17:31:13Z

services/customer/test/unit/test.ts

+
+let custCtrl: CustomerController;
+
+// const testCust = {


Make sure to delete these comments before merging

shimks · 2018-03-27T17:33:36Z

services/facade/src/controllers/account.management.controller.api.ts

+            content: {
+              '*/*': {
+                schema: {
+                  $ref: '#/components/schemas/AccountFull',


Did you mean Account?

shimks · 2018-03-27T17:35:25Z

services/facade/src/controllers/account.management.controller.ts

+  transactionRepository: TransactionRepository;
+
+  constructor() {
+    this.accountRepository = new AccountRepository();


Can we use dependency injections for these instead?

shimks · 2018-03-27T17:36:14Z

services/facade/src/index.ts

@@ -0,0 +1,25 @@
+import {FacadeMicroservice} from './application';
+import {RestServer, RestBindings} from '@loopback/rest';
+import 'loopback-connector-swagger';


No need for this line

shimks · 2018-03-27T17:38:16Z

services/facade/src/repositories/account/account.repository.api.ts

@@ -0,0 +1,76 @@
+export const accountDefinition = {
+  swagger: '2.0',


I'm not too caught up on the purpose of facade, but is it necessary for the package to be using loopback-connector-swagger @strongloop/lb-next-dev ?

shimks · 2018-04-05T20:56:05Z

@thinusn Would you still be interested in doing further work on this PR? I personally think it is good to land on its own. Please let us know so that we could move further on ahead with this repository :)

thinusn · 2018-04-06T10:36:02Z

@shimks I think it is about time we get this pr merged, would rather do more in a future pr

shimks · 2018-04-06T15:53:38Z

@thinusn I want to get the CI passing before we merge this PR and wanted to clean the PR up a bit to get it to pass. Could you grant me permission to make changes to the PR?

shimks · 2018-04-09T20:00:11Z

@slnode test please

shimks · 2018-04-09T21:05:36Z

Thanks @thinusn for your massive help in updating this hugely outdated repository :D

thinusn requested review from b-admike, bajtos, dhmlau, jannyHou, kjdelisle, shimks and virkt25 as code owners November 5, 2017 12:52

b-admike requested changes Nov 6, 2017

View reviewed changes

bajtos mentioned this pull request Feb 8, 2018

Decouple wiring of implementation bits from application configuration loopbackio/loopback-next#742

Closed

20 tasks

b-admike mentioned this pull request Feb 9, 2018

Drop Node.js 6 support #80

Merged

dhmlau mentioned this pull request Feb 18, 2018

Review existing documentation loopbackio/loopback-next#988

Closed

46 tasks

kjdelisle suggested changes Mar 7, 2018

View reviewed changes

jannyHou requested changes Mar 8, 2018

View reviewed changes

thinusn commented Mar 10, 2018

View reviewed changes

b-admike reviewed Mar 22, 2018

View reviewed changes

thinusn commented Mar 23, 2018

View reviewed changes

shimks reviewed Mar 27, 2018

View reviewed changes

refactor: update to work with latest lb

f5a746e

shimks mentioned this pull request Apr 9, 2018

Finish updating this repository #94

Open

shimks merged commit 70c55fc into strongloop:master Apr 9, 2018

shimks deleted the update-exaple-to-use-latest-loopback-versions branch April 9, 2018 21:04

bajtos mentioned this pull request Apr 17, 2018

Add @thinusn as a maintainer #96

Closed

dhmlau mentioned this pull request Apr 17, 2018

example doesn't run #76

Open

		@@ -0,0 +1,4 @@
		import {Entity, model} from '@loopback/repository';

		@model(require('./accoun-definition.json'))

		@@ -2,11 +2,10 @@ import {Entity, model, ModelDefinition} from '@loopback/repository';

		@model(require('./account/model-definition'))

		@@ -0,0 +1,76 @@
		export const accountDefinition = {
		swagger: '2.0',

Update examples to work with latest loopback versions #78

Update examples to work with latest loopback versions #78

Conversation

thinusn commented Nov 5, 2017 • edited Loading

Description

Related issues

Checklist

slnode commented Nov 5, 2017

b-admike 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 commented Feb 5, 2018

b-admike commented Feb 6, 2018 • edited Loading

dhmlau commented Feb 18, 2018

b-admike commented Mar 7, 2018 • edited Loading

kjdelisle 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

jannyHou 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

b-admike commented Mar 15, 2018

b-admike commented Mar 20, 2018

thinusn commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike commented Mar 22, 2018

thinusn commented Mar 23, 2018

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

shimks left a comment • edited Loading

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

shimks commented Apr 5, 2018

thinusn commented Apr 6, 2018

shimks commented Apr 6, 2018

shimks commented Apr 9, 2018

shimks commented Apr 9, 2018

thinusn commented Nov 5, 2017 •

edited

Loading

b-admike commented Feb 6, 2018 •

edited

Loading

b-admike commented Mar 7, 2018 •

edited

Loading

thinusn commented Mar 21, 2018 •

edited

Loading

shimks left a comment •

edited

Loading