feat(cli): add lb4 relation command #2426

oleg269 · 2019-02-19T15:56:39Z

Implementing add controller for relation; updating models using ast; implementing hasMany and
belongsTo relations. Still missing updating repository.

#1359

Checklist

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

The-Murdock · 2019-02-19T16:03:22Z

this is still WIP, waiting for comments on what we achieved so far.

dhmlau · 2019-02-26T15:58:08Z

@strongloop/loopback-maintainers, PTAL.

marioestradarosa

@oleg269 this is a very important contribution for the developer experience. I left some comments on typos an a proposal (not mandatory ) to list repositories, but so far LGTM 👍 .

packages/cli/generators/relation/relationutils.js

packages/cli/generators/relation/relationBelongsTo.js

elv1s

I manually tested this and it works as expected. Both hasMany and belongsTo update the models and create controllers.

The controllers don't transpile yet because the repository needs to be updated, but this is known.

Great work!

packages/cli/generators/relation/relationutils.js

elv1s · 2019-02-26T21:41:34Z

another minor comment; in the pull request comment markdown, a checkbox is marked completed with 'x', or space, instead of 'v' or no space. This will render the checklist correctly.

oleg269 · 2019-02-27T15:55:53Z

@elv1s
Updated pull request comment.

bajtos

I quickly skimmed through the pull request, don't see any obvious problems beyond missing test coverage for different relation types. Please add more tests to ensure all supported relation types are executed & covered.

packages/cli/generators/relation/relation.js

packages/cli/generators/relation/relationBelongsTo.js

packages/cli/generators/relation/relationHasOne.js

packages/cli/generators/relation/relationutils.js

packages/cli/test/integration/generators/relation.integration.js

packages/cli/test/integration/generators/model.relation.integration.js

bajtos · 2019-03-01T14:13:05Z

I run lb4 repository for hasMany relation (Category has many Product instances). I am pretty impressed with the results!

I discovered two problems though:

(1)
The code manipulating TS files is changing indentation levels and using incorrect quotation style.

+import { Product } from "./product.model";

 @model()
 export class Category extends Entity {
-  @property({
-    type: 'number',
-    id: true,
-    required: true,
-  })
-  id: number;
+    @property({
+        type: 'number',
+        id: true,
+        required: true,
+    })
+    id: number;

Is there a way how to configure ts-simple-ast to honor existing code style? If not, then can we at least tell it to use 2 spaces for indentation, use single quotes and don't put spaces around curly braces? Alternatively, perhaps we could run prettier to fix formatting of the modified files. I vaguely remember we have been discussing this in the past, maybe other generators are already running prettier? I am not sure. @raymondfeng @b-admike @jannyHou do you perhaps remember?

(2)
The generator does not modify the repository class to include HasManyRepository factory. As a result, the controller code does not compile.

return await this.categoryRepository.products(id).find(filter);
// Property 'products' does not exist on type 'CategoryRepository'.ts(2339)

It would be great to add one test for each relation type where we will build the project after a relation was defined, to verify that we are scaffolding valid code.

packages/cli/generators/relation/index.js

packages/cli/generators/relation/relationBelongsTo.js

packages/cli/generators/relation/relationutils.js

b-admike

Thank you for your contribution @oleg269! I did a trial run and since this is WIP, I understand that what I'm pointing out might be implemented soon, so as a reminder, I do not see the repository files changed after running lb4 relation for a Customer hasMany Order scenario. I assume more tests are to follow as well.

EDIT: Just realized @bajtos mentioned what I'm talking about 👍

docs/site/Relation-generator.md

packages/cli/package.json

packages/cli/generators/relation/index.js

oleg269 · 2019-03-12T13:59:54Z

@bajtos
Added run prettier to fix formatting of the modified files.
Added to generators code to modify the repository classes.

oleg269 · 2019-03-12T17:30:14Z

Hi,

new commit:

Added updating repositories using ast;
added tests;
resolved all reviews from this PR.

docs/site/Relation-generator.md

bajtos

Great progress!

Besides the comments below: What is the purpose of an empty file packages/cli/test/fixtures/relation/controllers/index.ts? Can we remove it please?

docs/site/Relation-generator.md

packages/cli/generators/relation/templates/controller-relation-template-belongs-to.ts.ejs

packages/cli/generators/relation/templates/controller-relation-template-has-many.ts.ejs

packages/cli/package.json

oleg269 · 2019-03-17T06:31:32Z

Empty file packages/cli/test/fixtures/relation/controllers/index.ts was removed.

oleg269 · 2019-03-19T09:02:35Z

Hi,

I finished implementation lb4 relation command.
Please review the code.

docs/site/Relation-generator.md

packages/cli/package.json

bajtos

Almost there!

packages/cli/generators/relation/utils.generator.js

dhmlau · 2019-04-01T17:58:39Z

@oleg269, seems like your PR is good to go. Could you please rebase and squash the commits? then we'll help you land. Thanks!!

b-admike · 2019-04-10T15:05:10Z

packages/cli/test/integration/cli/cli.integration.js

-        'lb4 openapi\n  lb4 observer',
-=======
-        'lb4 openapi\n  lb4 relation',
->>>>>>> c589f34... feat(cli): add lb4 relation command


FYI I squashed this fix in the commit because I forgot to remove these diff lines when rebasing.

emonddr · 2019-04-11T20:12:21Z

The strings are found in : /loopback-next/packages/cli/node_modules/yeoman-generator/lib/index.js in the Generator class :

    // Determine the app root
    this.contextRoot = this.env.cwd;

    let rootPath = findUp.sync('.yo-rc.json', {
      cwd: this.env.cwd
    });
    rootPath = rootPath ? path.dirname(rootPath) : this.env.cwd;

    if (rootPath !== this.env.cwd) {
      this.log(
        [
          '',
          'Just found a `.yo-rc.json` in a parent directory.',
          'Setting the project root at: ' + rootPath
        ].join('\n')
      );
      this.destinationRoot(rootPath);
    }

    this.appname = this.determineAppname();
    this.config = this._getStorage();
    this._globalConfig = this._getGlobalStorage();

emonddr · 2019-04-11T20:17:24Z

With mocha reporter:"dot" removed, we can see which test was occurring when strings were printed:

=== ATTENTION - INVALID USAGE OF CONSOLE LOGS DETECTED ===

at ControllerGenerator.log (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/node_modules/yeoman-environment/lib/util/log.js:80:21)
at new Generator (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/node_modules/yeoman-generator/lib/index.js:137:12)
at new BaseGenerator (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/lib/base-generator.js:8:68)
at new ArtifactGenerator (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/lib/artifact-generator.js:6:66)
at new ControllerGenerator (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/generators/controller/index.js:7:66)
at Environment.instantiate (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/node_modules/yeoman-environment/lib/environment.js:429:12)
at Environment.create (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/node_modules/yeoman-environment/lib/environment.js:407:17)
at Object.exports.testSetUpGen (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/test/test-utils.js:18:14)
at Context.it (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/cli/test/integration/lib/base-generator.js:17:31)
at callFn (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runnable.js:387:21)
at Test.Runnable.run (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runnable.js:379:7)
at Runner.runTest (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runner.js:535:10)
at /Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runner.js:653:12
at next (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runner.js:447:14)
at /Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runner.js:457:7
at next (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runner.js:362:14)
at Immediate._onImmediate (/Users/dremond/Documents/PRs_LoopBack/2426_again1/loopback-next/packages/build/node_modules/mocha/lib/runner.js:425:5)
at runCallback (timers.js:705:18)
at tryOnImmediate (timers.js:676:5)
at processImmediate (timers.js:658:5)

emonddr · 2019-04-11T20:37:14Z

Still investigating...

emonddr · 2019-04-12T03:28:15Z

By itself, this one succeeds...

emonddr · 2019-04-12T17:37:03Z

I tried running certain tests individually .
I added different mocha test entries into my package.json scripts section.

    "mocha_hasConsoleLogError8": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/extension.integration.js\" ",
    "mocha_hasConsoleLogError7": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/observer.integration.js\" ",
    "mocha_hasConsoleLogError6": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/openapi-petstore.integration.js\" ",
    "mocha_hasConsoleLogError5": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/openapi-uspto-with-anonymous.integration.js\" ",
    "mocha_hasConsoleLogError4": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/openapi-uspto.integration.js\" ",
    "mocha_hasConsoleLogError3": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/repository.integration.js\" ",
    "mocha_hasConsoleLogError1": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/swagger-petstore.integration.js\" ",
    "mocha_hasConsoleLogError2": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/service.integration.js\" ",
    "mocha_passes7": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/hasmany.relation.integration.js\" ",
    "mocha_passes6": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/hasmany.relation.integration.js\" ",
    "mocha_passes5": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/belongsto.relation.integration.js\" ",
    "mocha_passes4": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/model.integration.js\" ",
    "mocha_some_failures_versions": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/example.integration.js\" ",
    "mocha_passes3": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/datasource.integration.js\" ",
    "mocha_passes2": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/controller.integration.js\" ",
    "mocha_passes1": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/app.integration.js\" ",

Any entry of the form mocha_passes{digit} was a successful run with no errors.
Any entry of the form mocha_hasConsoleLogError{digit} experienced the problem with console output.
Any entry of the form mocha_some_failures_versions failed due to some version tests.

You'll notice that

"mocha_passes6": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/hasmany.relation.integration.js\" ",
    "mocha_passes5": "node packages/build/bin/run-mocha  \"packages/cli/test/integration/generators/belongsto.relation.integration.js\" ",

run successfully.

So is it a non-test file contributing to the problem (not a mocha test file)? Or is it a combination of the new test files called within the suite of the existing test files?

@oleg269 , please look into this, thank you. :)

oleg269 · 2019-04-15T07:59:06Z

@bajtos @emonddr I'll check this error.

oleg269 · 2019-04-16T07:22:53Z

@bajtos @emonddr
The best-generator tests use current working directory.
After adding belongsto relation tests current working directory was changed to .sandbox and best-generator tests displayed message: 'found a .yo-rc.json in a parent directory'. I fixed best-generator tests: changed the current directory in which the file .yo-rc.json already exist.

bajtos

I fixed best-generator tests: changed the current directory in which the file .yo-rc.json already exist.

Sounds reasonable.

coverage/coveralls — Coverage decreased (-0.4%) to 90.844%

This is a significant drop in our code coverage. I have reviewed the coverage report, there are quite few meaningful scenarios that are not covered by our tests.

Besides the places pointed out by my comments below, there are many error edge cases handled by relation/index.js but not verified by any test. Please see the coverage report for that file here to find error scenarios needing a test.

Sorry for requesting further changes so late in the process. Unfortunately, no code coverage is reported when the test failed, that's why I was not aware of this problem earlier.

packages/cli/generators/relation/base-relation.generator.js

packages/cli/generators/relation/utils.generator.js

bajtos

@oleg269 nice improvements 👍

Some of my earlier comments seem to be not addressed yet. The code coverage is dropping by 0.1% now, I think there are few more error scenarios to cover by our tests.

For example:

https://github.com/strongloop/loopback-next/blob/d4300f70bd209f70aca4f3498bd12ffda2eda188/packages/cli/generators/relation/index.js#L323-L331

https://github.com/strongloop/loopback-next/blob/d4300f70bd209f70aca4f3498bd12ffda2eda188/packages/cli/generators/relation/index.js#L356-L358

I also see quite few places where you are calling this.prompt() and then catching any prompt errors to print a debug log and call this.exit(err). Are these catch block needed? Isn't it enough to let the error propagate through the promise chain up to Yeoman handler? See e.g. here:
https://github.com/strongloop/loopback-next/blob/d4300f70bd209f70aca4f3498bd12ffda2eda188/packages/cli/generators/relation/index.js#L352-L355

Last but not least: I see a lot of .then() blocks in your new code. I believe we can use async functions and await keyword in our CLI package, because we require Node.js 8+ and thus async/await support is always available. I guess it's late to rewrite the code you already have in your pull request, but could you please use async/await instead of .then() and .catch() in any future code you will write? Thanks 😄

packages/cli/generators/relation/index.js

packages/cli/test/integration/generators/negative-tests.relation.integration.js

oleg269 · 2019-04-21T18:09:30Z

@bajtos
All code reviews was fixed. Code coverage was increased (from -0.4% to +0.2%).
What is left to do for merging?

raymondfeng · 2019-04-21T18:29:44Z

@oleg269 Thank you for the update. I have rebased your branch to latest master from strongloop/loopback-next. Please reset your local repo to it.

raymondfeng · 2019-04-22T05:13:09Z

@oleg269 I'm not sure if your last push was intentional as it makes the PR be in conflict with the upstream again. Please note I had pushed the rebased result back to your remote repo before your last one.

raymondfeng · 2019-04-22T17:52:21Z

@oleg269 I have squashed and rebased the PR into one commit.

raymondfeng · 2019-04-29T14:33:19Z

@oleg269 Thank you very much for putting great efforts to create and refine this PR. It's a good starting point to help improve model relations. I'm trying the CLI by myself and would like to see if there are simpler ways to prompt questions.

I'm also trying lb relation to see if 3.x has a better flow.

raymondfeng

Kudos for @elv1s @TomerSalton to make it happen!

There is room for improvement in the developer experience. But it's a good starting point. I approve it and defer more refinement in future PRs.

bajtos · 2019-05-24T12:12:16Z

Landed, thank you for the awesome contribution ❤️

oleg269 requested review from bajtos and raymondfeng as code owners February 19, 2019 15:56

dhmlau added CLI community-contribution labels Feb 21, 2019

marioestradarosa changed the title ~~feat(cli): add lb4 relation command~~ [WIP] feat(cli): add lb4 relation command Feb 26, 2019

marioestradarosa reviewed Feb 26, 2019

View reviewed changes

packages/cli/generators/relation/relationutils.js Outdated Show resolved Hide resolved

packages/cli/generators/relation/relationutils.js Outdated Show resolved Hide resolved

packages/cli/generators/relation/relationBelongsTo.js Outdated Show resolved Hide resolved

elv1s reviewed Feb 26, 2019

View reviewed changes

packages/cli/generators/relation/relationutils.js Outdated Show resolved Hide resolved

bajtos reviewed Mar 1, 2019

View reviewed changes

packages/cli/generators/relation/index.js Outdated Show resolved Hide resolved

bajtos reviewed Mar 1, 2019

View reviewed changes

packages/cli/generators/relation/index.js Outdated Show resolved Hide resolved

bajtos reviewed Mar 1, 2019

View reviewed changes

packages/cli/generators/relation/relationBelongsTo.js Outdated Show resolved Hide resolved

bajtos reviewed Mar 1, 2019

View reviewed changes

packages/cli/generators/relation/relationutils.js Outdated Show resolved Hide resolved

bajtos mentioned this pull request Mar 7, 2019

cli: add lb4 relation command #2223

Closed

7 tasks

b-admike reviewed Mar 7, 2019

View reviewed changes

nabdelgadir reviewed Mar 12, 2019

View reviewed changes

docs/site/Relation-generator.md Outdated Show resolved Hide resolved

bajtos suggested changes Mar 15, 2019

View reviewed changes

oleg269 changed the title ~~[WIP] feat(cli): add lb4 relation command~~ feat(cli): add lb4 relation command Mar 19, 2019

bajtos reviewed Mar 19, 2019

View reviewed changes

docs/site/Relation-generator.md Outdated Show resolved Hide resolved

packages/cli/package.json Outdated Show resolved Hide resolved

bajtos suggested changes Mar 19, 2019

View reviewed changes

packages/cli/generators/relation/utils.generator.js Outdated Show resolved Hide resolved

bajtos requested review from elv1s and removed request for elv1s March 19, 2019 11:15

nabdelgadir approved these changes Mar 29, 2019

View reviewed changes

dhmlau mentioned this pull request Apr 1, 2019

Monthly Milestone - April 2019 🌿🐰☔️ #2669

Closed

46 tasks

emonddr added this to the April 2019 milestone milestone Apr 1, 2019

b-admike reviewed Apr 10, 2019

View reviewed changes

bajtos reviewed Apr 16, 2019

View reviewed changes

bajtos mentioned this pull request Apr 18, 2019

New syntax for INDEX and FOREIGN KEY definition #2766

Open

5 tasks

bajtos reviewed Apr 18, 2019

View reviewed changes

dhmlau mentioned this pull request Apr 30, 2019

Monthly Milestone - May 2019 🌷🐝 🍄 #2818

Closed

32 tasks

bajtos modified the milestones: April 2019 milestone, May 2019 milestone May 2, 2019

raymondfeng approved these changes May 15, 2019

View reviewed changes

feat(cli): add lb4 relation command

e481efa

bajtos merged commit 75939a4 into loopbackio:master May 24, 2019

bajtos mentioned this pull request May 24, 2019

[CLI] add lb4 relation command #1359

Closed

16 tasks

deepakrkris mentioned this pull request May 7, 2020

Use AST instead of ejs templates for generators #2113

Closed

bajtos mentioned this pull request May 22, 2020

@loopback/cli: Generate proper relation belongsTo fix #5430 #5492

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add lb4 relation command #2426

feat(cli): add lb4 relation command #2426

oleg269 commented Feb 19, 2019 •

edited by bajtos

Loading

The-Murdock commented Feb 19, 2019

dhmlau commented Feb 26, 2019

marioestradarosa left a comment

elv1s left a comment

elv1s commented Feb 26, 2019

oleg269 commented Feb 27, 2019

bajtos left a comment

bajtos commented Mar 1, 2019 •

edited

Loading

b-admike left a comment •

edited

Loading

oleg269 commented Mar 12, 2019

oleg269 commented Mar 12, 2019 •

edited

Loading

bajtos left a comment

oleg269 commented Mar 17, 2019

oleg269 commented Mar 19, 2019

bajtos left a comment

dhmlau commented Apr 1, 2019

b-admike Apr 10, 2019

emonddr commented Apr 11, 2019

emonddr commented Apr 11, 2019 •

edited

Loading

emonddr commented Apr 11, 2019

emonddr commented Apr 12, 2019

emonddr commented Apr 12, 2019

oleg269 commented Apr 15, 2019

oleg269 commented Apr 16, 2019

bajtos left a comment

bajtos left a comment

oleg269 commented Apr 21, 2019

raymondfeng commented Apr 21, 2019

raymondfeng commented Apr 22, 2019

raymondfeng commented Apr 22, 2019

raymondfeng commented Apr 29, 2019

raymondfeng left a comment

bajtos commented May 24, 2019

feat(cli): add lb4 relation command #2426

feat(cli): add lb4 relation command #2426

Conversation

oleg269 commented Feb 19, 2019 • edited by bajtos Loading

Checklist

The-Murdock commented Feb 19, 2019

dhmlau commented Feb 26, 2019

marioestradarosa left a comment

Choose a reason for hiding this comment

elv1s left a comment

Choose a reason for hiding this comment

elv1s commented Feb 26, 2019

oleg269 commented Feb 27, 2019

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Mar 1, 2019 • edited Loading

b-admike left a comment • edited Loading

Choose a reason for hiding this comment

oleg269 commented Mar 12, 2019

oleg269 commented Mar 12, 2019 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

oleg269 commented Mar 17, 2019

oleg269 commented Mar 19, 2019

bajtos left a comment

Choose a reason for hiding this comment

dhmlau commented Apr 1, 2019

b-admike Apr 10, 2019

Choose a reason for hiding this comment

emonddr commented Apr 11, 2019

emonddr commented Apr 11, 2019 • edited Loading

emonddr commented Apr 11, 2019

emonddr commented Apr 12, 2019

emonddr commented Apr 12, 2019

oleg269 commented Apr 15, 2019

oleg269 commented Apr 16, 2019

bajtos left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

oleg269 commented Apr 21, 2019

raymondfeng commented Apr 21, 2019

raymondfeng commented Apr 22, 2019

raymondfeng commented Apr 22, 2019

raymondfeng commented Apr 29, 2019

raymondfeng left a comment

Choose a reason for hiding this comment

bajtos commented May 24, 2019

oleg269 commented Feb 19, 2019 •

edited by bajtos

Loading

bajtos commented Mar 1, 2019 •

edited

Loading

b-admike left a comment •

edited

Loading

oleg269 commented Mar 12, 2019 •

edited

Loading

emonddr commented Apr 11, 2019 •

edited

Loading