feat(cli): generate controller with CRUD methods #849

kjdelisle · 2018-01-09T18:50:28Z

Description

CLI command for generating controllers now supports the creation of a CRUD controller template.

You will now receive a prompt to select which kind of controller to generate
If you select "Basic CRUD controller", it will scan for models and repositories, then prompt you to select an applicable model/repository
If no models or repositories exist in the appropriate directories, you will receive an error.

Implements #727 (Part 1 of 2)

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

kjdelisle · 2018-01-09T19:09:52Z

The tests are currently failing in Node 6 due to lack of the util.promisify API (which is Node 8+).
I know we're planning to move away from 6, but I think I'm going to refactor this to use a different promisification tool instead of blocking this PR.

shimks

LGTM. You might want to ping @virkt25 so that you can get him to merge #778. His changes should only affect your setupTmpDir (I think).

raymondfeng · 2018-01-09T22:09:00Z

packages/cli/test/test-utils.js

+ * structure.
+ * @param {string} tmpDir Path to the temporary directory to setup
+ */
+exports.setupTmpDir = function(tmpDir) {


This function should be from @loopback/workspace in the future to create a dummy project.

Please use a more descriptive name. setupTmpDir does not tell us anything about what is being set up in the target directory. I am proposing to use givenAnApplicationDir or setupAProjectDir or similar.

+1 for givenAnApplicationDir

raymondfeng · 2018-01-09T22:51:26Z

packages/cli/generators/controller/index.js

+ * Not a member of the class because Yeoman will always call Generator
+ * prototype functions! Use .apply or .call with this function!
+ */
+function promptArtifactCrudVars() {


We should make it a member function and check this.artifactInfo.controllerType === ControllerGenerator.CRUD to proceed.

raymondfeng · 2018-01-09T22:51:55Z

packages/cli/generators/controller/index.js

+    })
+    .then(list => {
+      if (_.isEmpty(list)) {
+        return Promise.reject(


We should call this.exit(error) to gracefully exit.

raymondfeng · 2018-01-09T22:53:10Z

packages/cli/generators/controller/index.js

+          self.artifactInfo.modelNameCamel = utils.camelCase(
+            self.artifactInfo.modelName
+          );
+          return Promise.resolve(props);


No need to wrap props into a promise. A normal return value for then() will be converted to a promise automatically.

raymondfeng · 2018-01-09T22:54:17Z

packages/cli/generators/controller/templates/src/controllers/controller-crud-template.ts

+export class <%= name %>Controller {
+  @inject('repositories.<%= modelNameCamel %>') <%= repositoryNameCamel %> : <%= repositoryName %>;
+  async create(obj: <%= modelName %>) : Promise<<%= modelName %>> {
+    return this.<%= repositoryNameCamel %>.create(obj);


It should be return await this.<%= repositoryNameCamel %>.create(obj);. The other option is to remove async keyword as no await is not in the method body.

See https://github.com/strongloop/loopback-next/blob/c5ad28a60003e9928158db4acb8a68686ef31c35/packages/repository-rest/src/controllers/crud-controller.ts

raymondfeng · 2018-01-09T22:56:35Z

packages/cli/lib/artifact-generator.js

-
    return this.prompt(prompts).then(props => {
      Object.assign(this.artifactInfo, props);
+      return Promise.resolve(props);


No need to wrap props.

raymondfeng · 2018-01-09T22:57:20Z

packages/cli/lib/utils.js

+ * paths. Must return a Promise.
+ * @returns {Promise<string[]>} The filtered list of paths.
+ */
+exports.findArtifactPaths = function(path, artifactType, reader) {


These functions should be from @loopback/workspace in the future.

raymondfeng · 2018-01-09T22:58:53Z

packages/cli/generators/controller/index.js

+    .getArtifactList(self.artifactInfo.modelDir, 'model')
+    .then(list => {
+      if (_.isEmpty(list)) {
+        return Promise.reject(


We should call this.exit(error) to gracefully exit.

It seems that existence of models is required for lb4 controller to be useful. We probably need to add lb4 model soon.

raymondfeng

Please address my comments.

raymondfeng · 2018-01-09T23:08:12Z

packages/cli/generators/controller/index.js

+            self.artifactInfo.modelName,
+            self.artifactInfo.repositoryName,
+          ].forEach(item => {
+            const validationMsg = utils.validateClassName(item);


We should set up validate function for each of the prompts instead of checking after all answers are accepted. The prompt level validator will reject invalid inputs before the next one. See an example at https://github.com/strongloop/loopback-next/blob/master/packages/cli/generators/app/index.js#L45.

kjdelisle · 2018-01-10T21:17:07Z

@slnode test please (not sure why these jobs keep dropping...)

raymondfeng · 2018-01-10T21:50:22Z

packages/cli/generators/controller/index.js

+          ].forEach(item => {
+            const validationMsg = utils.validateClassName(item);
+            if (typeof validationMsg === 'string')
+              throw new Error(validationMsg);


We should set up validate function for each of the prompts instead of checking after all answers are accepted. The prompt level validator will reject invalid inputs before the next one. See an example at https://github.com/strongloop/loopback-next/blob/master/packages/cli/generators/app/index.js#L45.

kjdelisle · 2018-01-11T14:03:20Z

@slnode test please

bajtos · 2018-01-11T15:05:54Z

packages/cli/generators/controller/index.js

+            type: 'list',
+            name: 'id',
+            message: 'What is the type of your ID?',
+            choices: ['number', 'string', 'object'],


Ideally, we should be able to the type of model's id property automatically. Just saying, an explicit prompt is fine with me for MVP.

We might need to build a separate module for the code that constructs the runtime type metadata so that we can share it across the juggler/relations, the openapi-spec and the CLI (since they're all going to need to know this information for some purpose).

bajtos

Great stuff! I did not review all details, I focused on the most important parts only.

bajtos · 2018-01-11T15:11:15Z

packages/cli/generators/controller/templates/src/controllers/controller-crud-template.ts

+import {<%= repositoryName %>} from '../repositories';
+
+export class <%= name %>Controller {
+  @inject('repositories.<%= modelNameCamel %>') <%= repositoryNameCamel %> : <%= repositoryName %>;


IMO, we should be using constructor injection, because we don't have any reasonable default to use for this repository if the user of our API does not provide us any. We are advocating for this approach in our best practices too, see http://loopback.io/doc/en/lb4/Implementing-features.html#decouple-controller-from-repository

Further reading: https://social.msdn.microsoft.com/Forums/office/en-US/7a1dbdd5-ddf2-4eb2-8063-a423bb441158/when-to-use-property-injection-vs-constructor-injection?forum=csharpgeneral

If the class itself has a strong dependency upon the interface/object that you inject it with, you should use constructor injection as the class is pretty much useless without the dependency, i.e. it makes no sense to instantiate it without it. A constructor therefore enforces the dependency requirement.

But if it makes sense for the class to be able to do its job without using the dependency, you could use property injection. When using property injection the dependency may or may not used depending on whether the property is actually invoked.

So it depends.

Please refer to the following threads for more information about this:
http://stackoverflow.com/questions/1503584/dependency-injection-through-constructors-or-property-setters
http://stackoverflow.com/questions/21218868/setter-injection-vs-constructor-injection
http://programmers.stackexchange.com/questions/300706/dependency-injection-field-injection-vs-constructor-injection

bajtos · 2018-01-11T15:13:35Z

packages/cli/package.json

    "yeoman-test": "^1.7.0"
  },
  "dependencies": {
+    "@types/util.promisify": "^1.0.0",


This shouldn't be needed, CLI is written in vanilla JavaScript.

See also our existing packages/core/src/promisify.ts.

bajtos · 2018-01-11T15:16:33Z

packages/cli/test/test-utils.js

+ * structure.
+ * @param {string} tmpDir Path to the temporary directory to setup
+ */
+exports.setupTmpDir = function(tmpDir) {


Please use a more descriptive name. setupTmpDir does not tell us anything about what is being set up in the target directory. I am proposing to use givenAnApplicationDir or setupAProjectDir or similar.

bajtos · 2018-01-11T15:36:35Z

packages/cli/lib/utils.js

 'use strict';

+// TODO(kjdelisle): Drop this shim once we drop Node 6.
+require('util.promisify/shim')();


So AFAICT, util.promisify/shim.js is modifying Node.js built-in util export - that's not good, but easy to fix by using util.promisify/implementation instead. It's also trying to support kCustomPromisifiedSymbol, but I don't think that will work unless the shim is loaded as the first thing in the app, so that 3rd party modules have access to this symbol. I guess that does not matter much since we are going to drop support for Node.js 6.x soon anyways.

As I am thinking about this, I guess as long as you use implementation instead of shim, so that we don't modify built-in require('util') then I am fine with your change. Please consider updating the existing promisify function in core to use the same implementation under the hood, so that we have consistent behaviour. Preferably as part of your PR (see also mine https://github.com/strongloop/loopback-next/pull/848/files#diff-9dab89a9fdf9229c03e541d55747f3bf)

bajtos · 2018-01-11T15:37:06Z

packages/cli/test/utils.js

+          path.join('tmp', 'app', 'models', 'bar.js'),
+          path.join('tmp', 'app', 'models', 'README.js'),
+        ].concat(expected);
+        return Promise.resolve(result);


Why is it necessary to wrap the result in a promise? Can we fix utils.findArtifactsPaths to call Promise.resolve for us?

bajtos · 2018-01-11T15:37:07Z

packages/cli/test/utils.js

+        return Promise.resolve(files);
+      };
+      const artifactPath = path.join('tmp', 'app', folder);
+      it(name, () => {


My experience with tests generated like this is mixed. Because it is called from a helper function, the (async) stack trace reported by a failed test does not show the place where the test suite was defined (where are the test parameters specified).

Can we refactor this helper to remove the call to it?

I am envisioning usage like this:

describe('getArtifactList', () => { // ... it('finds JS repositories', () => { return verifyArtifactList(artifactType, folder, suffix, files, expected); }); // ... });

I think it would be even better if we could move expect(results).to.eql(expected) out of this helper function too. That way the helper function is focused on executing getArtifactList in the given scenario, and everything else is up to the individual test to handle.

bajtos · 2018-01-11T15:37:07Z

packages/cli/test/controller.js

+            utils.setupTmpDir(dir);
+          })
+          .withPrompts(withInputProps)
+          .toPromise();


This helper "before" hook seems to be duplicated at least in two places. Could you please extract it to a top-level helper shared by all describe suites?

bajtos

The updated code looks mostly good, please see few more comments below. Feel free to land the patch without waiting for another review from me, as long as somebody else approves your final version.

bajtos · 2018-01-12T16:04:04Z

packages/cli/lib/artifact-generator.js

        validate: utils.validateClassName,
      },
    ];
-


Nitpick: could you please preserve this empty line as a visual delimiter?

bajtos · 2018-01-12T16:07:04Z

packages/core/src/promisify.ts

 import * as util from 'util';
+// The @types/util.promisify conflicts with @types/node due to rescoping
+// issues, so falling back to legacy import.
+const promisifyPolyfill = require('util.promisify').implementation;


I think it's better to require('util.promisify/implementation')?

bajtos · 2018-01-12T16:09:18Z

packages/cli/lib/utils.js

 const regenerate = require('regenerate');
 const _ = require('lodash');
 const pascalCase = require('change-case').pascalCase;
+const promisify = require('util.promisify/implementation');


Oh, sorry for a misleading comment. Your code here will use the user-land promisify implementation even on Node.js 8.x where a native version is available.

Here is a better solution:

const promisify = util.promisify || require('util.promisify/implementation'); // ... exports.promisify = promisify;

(I am proposing to export the promisify helper so that I can easily access it in my #848 too).

bajtos · 2018-01-12T16:10:44Z

packages/cli/test/controller.js

-          path.join(tmpDir, 'package.json'),
-          JSON.stringify({
-            keywords: ['foobar'],
+    return runGenerator(true, true).then(() => {


Huh, what does true, true mean and how does it differ from false, true? Boolean arguments are evil, please used an options object with named properties.

runGenerator({useTempDir: true, withPrompts: true})

I knew I should've gone with my gut instinct!

bajtos · 2018-01-12T16:13:24Z

packages/cli/test/controller.js

-            JSON.stringify({
-              keywords: ['loopback'],
-            })
+            path.join(tmpDir, 'src', 'repositories', 'bar.repository.ts'),


Have you considered extracting a helper function for this? e.g.

givenRepositoryScript('bar.repository.ts'); givenModelScript('foo.model.ts');

bajtos · 2018-01-12T16:14:41Z

packages/cli/test/controller.js

+    if (useTempDir) {
+      result = result.inTmpDir(dir => {
+        utils.givenAnApplicationDir(dir);
+      });


result = result.inTmpDir(util.givenAnApplicationDir);

Unless givenAnApplicationDir is assuming this = utils?

bajtos · 2018-01-12T16:15:40Z

packages/cli/test/controller.js

 const helpers = require('yeoman-test');
 const fs = require('fs');
+const util = require('util');
+const utils = require('./test-utils');


The difference between util vs. utils is easy to overlook. Could you please rename the second import from utils to testUtils?

jannyHou · 2018-01-12T17:30:19Z

packages/cli/generators/controller/index.js

+    let modelList = [];
+    let repositoryList = [];
+    if (
+      !this.artifactInfo.controllerType ||


hmm, is promptArtifactCrudVars for "Basic CRUD controller" only or for any other controllers except artifactInfo.controllerType === ControllerGenerator.BASIC?

I'm glad you noticed. I wrote it this way since I'll be adding the REST CRUD Controller afterwards.

If in the future we have variance between which controllers do and don't require models/repositories/other artifacts, then we'll need to implement some sort of strategy pattern here to determine requirements

kjdelisle · 2018-01-12T18:23:36Z

@raymondfeng PT(another)L, I think it's almost ready to land.

kjdelisle · 2018-01-12T18:55:54Z

@slnode test please

raymondfeng · 2018-01-15T17:26:13Z

packages/cli/generators/controller/index.js

+    }
+    return utils
+      .getArtifactList(this.artifactInfo.modelDir, 'model')
+      .then(list => {


We need to add catch here so that errors from getArtifactList will be caught for graceful exit. For example, if there is no models directory, running the generator throws the following error:

events.js:183 throw er; // Unhandled 'error' event ^ Error: ENOENT: no such file or directory, scandir '/Users/rfeng/Projects/demos/x2/src/models'

I've added a .catch at the end of the chain to call this.exit (it won't mark this comment as outdated)

Unfortunately it does not work as your added catch only applies to return this.prompt([. I still see the same issue with your changes.

raymondfeng · 2018-01-15T21:57:42Z

@kjdelisle Some of the assertions fail on Windows. Please fix them.

1) lb4 controller
       CRUD
         fails
           when no model directory is present:
     AssertionError: expected 'ENOENT: no such file or directory, scandir \'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\109bfa9dc9ef1a20f4038382664ae090bbbcb12f\\src\\models\'' to match /ENOENT: no such file or directory, scandir(.*?)\/models/
      at Assertion.fail (packages\testlab\node_modules\should\as-function.js:275:17)
      at Assertion.value [as match] (packages\testlab\node_modules\should\as-function.js:356:19)
      at checkExitMessage (packages\cli\test\controller.js:241:43)
      at helpers.run.inTmpDir.withPrompts.on.then (packages\cli\test\controller.js:203:13)
      at <anonymous>
  2) lb4 controller
       CRUD
         fails
           when no repository directory is present:
     AssertionError: expected 'ENOENT: no such file or directory, scandir \'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\2f5838a2de3fc48df4e4b1067a24419fc7d7ad9c\\src\\repositories\'' to match /ENOENT: no such file or directory, scandir(.*?)\/repositories/
      at Assertion.fail (packages\testlab\node_modules\should\as-function.js:275:17)
      at Assertion.value [as match] (packages\testlab\node_modules\should\as-function.js:356:19)
      at checkExitMessage (packages\cli\test\controller.js:241:43)
      at helpers.run.inTmpDir.withPrompts.on.then (packages\cli\test\controller.js:230:13)
      at <anonymous>

raymondfeng · 2018-01-15T22:53:51Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts

+
+  @post('/<%= modelNameCamel %>')
+  @param.body('obj', <%= modelName %>)
+  async create(obj: <%= modelName %>) : Promise<<%= modelName %>> {


Please fix all methods to use parameter level decorators.

async create(@param.body('obj', <%= modelName %>) obj: <%= modelName %>) : Promise<<%= modelName %>> { return await this.<%= repositoryNameCamel %>.create(obj); }

kjdelisle · 2018-01-15T23:27:48Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts


  constructor(
-    @inject('repositories.<%= modelNameCamel %>')
+    @inject('repositories.<%= repositoryName %>')


Quick note: Was testing this on a real application and noticed that we must've changed our approach awhile ago, since the RepositoryMixin is now creating repositories with the name of the constructor object. Updated the template to reflect this so that it works out-of-the-box.

kjdelisle requested review from bajtos and raymondfeng as code owners January 9, 2018 18:50

kjdelisle added needs-priority and removed needs-priority labels Jan 9, 2018

kjdelisle self-assigned this Jan 9, 2018

kjdelisle force-pushed the cli/controller-with-crud branch from 4c633c5 to 44eed9f Compare January 9, 2018 18:52

kjdelisle requested a review from shimks as a code owner January 9, 2018 19:06

shimks approved these changes Jan 9, 2018

View reviewed changes

raymondfeng reviewed Jan 9, 2018

View reviewed changes

raymondfeng requested changes Jan 9, 2018

View reviewed changes

raymondfeng reviewed Jan 9, 2018

View reviewed changes

kjdelisle force-pushed the cli/controller-with-crud branch 2 times, most recently from 4e1f53b to 652ee9d Compare January 10, 2018 21:14

raymondfeng reviewed Jan 10, 2018

View reviewed changes

kjdelisle force-pushed the cli/controller-with-crud branch 2 times, most recently from 64a8fce to 7c78581 Compare January 10, 2018 22:47

bajtos reviewed Jan 11, 2018

View reviewed changes

bajtos suggested changes Jan 11, 2018

View reviewed changes

kjdelisle force-pushed the cli/controller-with-crud branch 2 times, most recently from 55e4bec to 38fbb8b Compare January 11, 2018 21:03

kjdelisle mentioned this pull request Jan 11, 2018

docs(lb4): scaffolding a CRUD controller loopbackio/loopback.io#583

Merged

bajtos reviewed Jan 12, 2018

View reviewed changes

bajtos approved these changes Jan 12, 2018

View reviewed changes

jannyHou reviewed Jan 12, 2018

View reviewed changes

kjdelisle force-pushed the cli/controller-with-crud branch 2 times, most recently from 3f2b195 to f5e883b Compare January 12, 2018 18:15

kjdelisle force-pushed the cli/controller-with-crud branch from f5e883b to 0c880b2 Compare January 15, 2018 15:53

b-admike approved these changes Jan 15, 2018

View reviewed changes

raymondfeng reviewed Jan 15, 2018

View reviewed changes

kjdelisle force-pushed the cli/controller-with-crud branch 3 times, most recently from 0487fe9 to ec52c3c Compare January 15, 2018 20:32

kjdelisle force-pushed the cli/controller-with-crud branch 3 times, most recently from ab81256 to 514debb Compare January 15, 2018 22:47

feat(cli): generate REST controller with CRUD methods

f7ce5cc

kjdelisle force-pushed the cli/controller-with-crud branch from 514debb to b83a0fb Compare January 15, 2018 22:48

refactor(core): use util.promisify to polyfill for Node 6

b83a0fb

raymondfeng reviewed Jan 15, 2018

View reviewed changes

fix(cli): rework template to use inline param decoration

304cd45

kjdelisle commented Jan 15, 2018

View reviewed changes

raymondfeng approved these changes Jan 15, 2018

View reviewed changes

kjdelisle merged commit e3ef86b into master Jan 15, 2018

kjdelisle deleted the cli/controller-with-crud branch January 15, 2018 23:50

kjdelisle removed the review label Jan 15, 2018

feat(cli): generate controller with CRUD methods #849

feat(cli): generate controller with CRUD methods #849

Uh oh!

Conversation

kjdelisle commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

kjdelisle commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shimks 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!

raymondfeng Jan 9, 2018 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjdelisle commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjdelisle commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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 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!

kjdelisle commented Jan 9, 2018 •

edited

Loading

kjdelisle commented Jan 9, 2018 •

edited

Loading

raymondfeng Jan 9, 2018 •

edited

Loading