Skip to content

Conversation

@marioestradarosa
Copy link
Contributor

@marioestradarosa marioestradarosa commented Aug 30, 2018

Done

  • Filtering out the soap and rest connectors from the data source list
  • Inferring the repository class (KV or CRUD) based on the connector property for JSON datasource
  • Allow multiple selection of models
  • Infers the id property name from the model Class based on 3 scenarios
    • The current template generated by lb4 model command
    • A JSON object inside the @model decorator
    • A JSON definition inside the class model
  • Prompts for the property name in case the infer process couldn't find the primary key property
  • Currently it is generating the repository based on the two repository types (KV or CRUD)
  • support for --config is inherited from the base generators
  • properly display an error message if it can't fine a src/datasources or src/models directory
  • Add basic documentation for the feature
  • Use name argument as the repository name
  • add shared functions in utils to define the path and artifact names for repositories and datasources
  • fixed lib/ArtifactGenerator to support multiple index files

Command line feature

  • Now we can supply the arguments from the command line. The arguments are checked against all the artifacts implemented already (ie: id inferring list, datasource list, model list) and if not valid, it falls back to the prompt.

Syntax: Use them interchangeably
lb4 repository name --datasource --model --id

Example:
lb4 repository --datasource dbmem --model=pluto --id=IDPK

It will generate the repository for pluto with id property name IDPK. It is checked if dbmem is a valid datasource, pluto (changed to Pluto) is a valid model and if its id can't be inferred then it falls back to use the id argument supplied. All validations are performed against the list of artifacts already loaded form their respective directories.

Test Status

  • Test on Default generated models using CRUD connector
  • Test on models defined by @model decorator using CRUD connector
  • Test on Default generated models using KV connector
  • Test on models defined by @model decorator using KV connector
  • Test on failing providing or selecting a model.
    • at this point the user should have selected a valid datasource, otherwise this point will never get reached
  • Test on multi-word models
  • Test on command line interface --model --id from a none-id inferred model file
  • Test failing when the datasource list is empty.
    • the datasources are not KV or CRUD, and thus it returns an empty list
    • or the datasource directory is empty
  • Add test for supporting config file
  • Add test for support of name argument
  • Add test for multiple repository generation

Preconditions checked

check the following directories in this order

  • check if the src/datasources exists, displaying an approriate error
  • check if the src/models exists

Implements #1588

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

@marioestradarosa marioestradarosa changed the title WIP feat(cli): repository generator WIP feat(cli): lb4 repository implementation Aug 30, 2018
marioestradarosa added a commit to marioestradarosa/loopback-next that referenced this pull request Aug 30, 2018
add lb4 repository to the cli integration test

implements loopbackio#1659
marioestradarosa added a commit to marioestradarosa/loopback-next that referenced this pull request Aug 30, 2018
add lb4 repository to the cli integration test

implements loopbackio#1659
@marioestradarosa marioestradarosa force-pushed the feat-add-lb-repository branch 3 times, most recently from 7590e27 to 8168b78 Compare August 31, 2018 04:15
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Nice start!

const utils = require('../../lib/utils');

const SERVICE_VALUE_CONNECTOR = 'soap,rest';
const KEY_VALUE_CONNECTOR = 'kv-';
Copy link
Member

Choose a reason for hiding this comment

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

Please use the "baseModel" property from the connector list used by the datasource generator to decide which connectors are CRUD, which are KeyValue and which are for Services.

  • baseModel value PersistedModel indicates a CRUD connector
  • baseModel value KeyValueModel indicates a KeyValue connector
  • baseModel value Model indicates a Service connector

The connector list is sourced from https://github.com/strongloop/loopback-workspace/blob/master/available-connectors.json and stored to generators/datasources/connectors.json, see https://github.com/strongloop/loopback-next/blob/ac011f80b229b5dd5c69851ab14b39705b852629/packages/cli/generators/datasource/index.js#L13

/** instance helper method isolated from the execution loop
* @connectorType: can be a single or a comma separated string list
*/
this.isConnectorType = async function(connectorType, dataSourceClassName) {
Copy link
Member

Choose a reason for hiding this comment

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

Helper methods should be defined as regular class methods and their name should start with _:

module.exports = class RepositoryGenerator extends ArtifactGenerator {
  constructor(args, opts) {
    // ...
  }

  private async _isConnectorType(connectorType, dataSourceClassName) {
    // ...
  }
}

{
type: 'list',
name: 'idType',
message: `What is the ID type of ${item} `,
Copy link
Member

Choose a reason for hiding this comment

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

Can you use typeof ${model}.prototype.id? See e.g. TodoRepository: https://github.com/strongloop/loopback-next/blob/ac011f80b229b5dd5c69851ab14b39705b852629/examples/todo/src/repositories/todo.repository.ts#L10-L13

In the future, we may want to ask the user for the name of the id property to use.

Regardless of that, I think we should not be duplicating the id type name in the repository class if possible.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will use typeof ${model}.prototype.id if the model has the id field and if it is set to the primary key. To do that, I am trying to see the best way to do it, probably just read the file and inspect it or use ATS to analyze it more in depth and even get the id name from it.

thoughts?

import {inject} from '@loopback/core';

export class <%= className %>Repository extends <%= repositoryTypeClass %><
<%= modelName %>, <%= idType %>> {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not going to pass prettier linter and should be changed to the following:

export class <%= className %>Repository extends <%= repositoryTypeClass %><
  <%= modelName %>, 
  <%= idType %>,
> {

@bajtos

This comment has been minimized.

@marioestradarosa
Copy link
Contributor Author

@bajtos I addressed your suggestions 👍 thanks.

PTAL

const chalk = require('chalk');
const utils = require('../../lib/utils');
const connectors = require('../datasource/connectors.json');
const tsquery = require('@phenomnomnominal/tsquery').tsquery;
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid tsquery is not properly licensed as open-source, which means we cannot add it to our dependencies.

I opened an issue in the project repository - see phenomnomnominal/tsquery#25, hopefully the project maintainers will fix it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, they added the License already 👍

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Most of the changes looks great! I have few concerns about the AST-based processing though.

return retArray;
}

async _getModelIdType(modelName) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please move the code detecting the "id" property into as standalone helper file that can be used by other generators? It will also make it easier to test this functionality.

A few non-obvious test cases that come to my mind:

The id property is defined via @model definition:

@model({
  name: 'Product',
  properties: {
    id: {type: 'number', id: true}
    name: {type: 'string', required: true},
  },
})
class Product extends Entity {
  id: number;
  name: string;
}

The model does not use any decorator:

class Product extends Entity {
  static definition = {
    name: 'Product',
    properties: {
      id: {type: 'number', id: true}
      name: {type: 'string', required: true},
    },
  };

  id: number;
  name: string;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's true, there are many variations. One that is a little bit more complex is when you import the JSON definition file from somewhere else (ie: from @yourcompany/models) . I believe we should tackle the most common scenarios and then fine tune it to land the PR?

Copy link
Member

Choose a reason for hiding this comment

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

I believe we should tackle the most common scenarios and then fine tune it to land the PR?

Yes please!

Just make sure there is a workaround solution for people using an uncommon scenario. I think your current implementation, where the user can enter a custom value if the AST-based reader did not find anything, is a great solution for that 👍


for (let connector of Object.values(connectors)) {
if (
jsonFileContent.connector === connector.name &&
Copy link
Member

Choose a reason for hiding this comment

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

I believe we allow two options how to specify a connector used by a datasource, at least that's the case of LB 3.x CLI and both LB 3.x/4.x runtime:

  1. Using the full name, e.g. loopback-connector-mysql.
  2. Using a short name without loopback-connector- prefix, i.e. mysql.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we should ask for both, short as of now and the longer version right?

Copy link
Member

@bajtos bajtos Sep 3, 2018

Choose a reason for hiding this comment

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

The prompt for a connector to use is handled by lb4 datasource command, no need to change that.

I am saying that you should improve this check to correctly handle both long and short form of the connector property specified in the datasource configuration.

E.g.

const connector = jsonFileContent.connector;

if (
  connector === connector.name ||
  `loopback-connector-${connector}` === connector.name
  // ...
) {
}

Also please refactor this multi-line if condition to fit into a single line, see Indentation of multi-line expressions in if, the example called "Best".

);
if (!result) {
// remove from original list
_.remove(datasourcesList, e => e == item);
Copy link
Member

Choose a reason for hiding this comment

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

Please rewrite this look using Array.prototype.filter function.

const availableDatasources = datasourcesList.filter(item => {
  const result = await this._isConnectorOfType(
     VALID_CONNECTORS_FOR_REPOSITORY,
     item,
   );
   debug(/*...*/);
   return result;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos , sure, I will update it, but what would be the benefit ? .

Copy link
Member

Choose a reason for hiding this comment

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

The code is easier to reason about because you are not modifying the array items in the middle of the loop iterating through it, and you also don't need _.remove from an external library.

}
}

if (_.isEmpty(datasourcesList)) {
Copy link
Member

Choose a reason for hiding this comment

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

How about using !datasourcesList.length, since we know the list always contain an array value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used _.isEmpty because it appears in the generators, but again what would be the benefit, is there an overhead for the lodash method?

name: 'propertyName',
message: `Please, select the ID property for ${item} from the list `,
choices: modelPropertyList,
default: modelPropertyList[0],
Copy link
Member

Choose a reason for hiding this comment

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

Can we perhaps leave AST-based detection of model properties out of the first version of lb4 repository to simplify the initial implementation and get it landed sooner?

I think the following prompt should be good enough for now:

const prompts = [
  {
    type: 'input',
    name: 'propertyName'
    message: `Please enter the name of the ID property of ${item}:`,
    default: 'id',
  },
];

Thoughts?

cc @raymondfeng @virkt25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if they already updated the license in their repo, shall we remove the AST-based detection for this PR ?

answer.propertyName
}`;
} else {
this.artifactInfo.idType = idType;
Copy link
Member

Choose a reason for hiding this comment

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

Please use typeof ${item}.prototype${idType} in this branch too.

The benefit is that when people rename models and/or change the id property type in their application later in the future, the Controller code will automatically track those changes.

When you put a hard-coded ID type in the generated controller source, users have to update that ID type when it changes, which can be tedious.

"yeoman-environment": "^2.0.6",
"yeoman-test": "^1.7.0"
"yeoman-test": "^1.7.0",
"typescript": "^3.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

I am curious, what do we need typescript for? Why is it a direct dev-dependency?

Copy link
Contributor Author

@marioestradarosa marioestradarosa Sep 4, 2018

Choose a reason for hiding this comment

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

@bajtos , they answered my issue and will fix it today. They will apply typescript as a peerDependcy rather than the current scenario devDependency.

@marioestradarosa

This comment has been minimized.

@marioestradarosa marioestradarosa force-pushed the feat-add-lb-repository branch 3 times, most recently from b97b84f to 95936ce Compare September 4, 2018 15:27
@@ -0,0 +1,149 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

The file name should be ast-helper.js, right? (ast => AST)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch 👍

@marioestradarosa marioestradarosa force-pushed the feat-add-lb-repository branch 2 times, most recently from 9ea81ea to 5af320d Compare September 5, 2018 00:58
@hacksparrow
Copy link
Contributor

How about just --model instead of --modelName, and --id instead of --idName?

@marioestradarosa
Copy link
Contributor Author

marioestradarosa commented Sep 13, 2018

I was able to generate and compile succesfully a KV repository, but I had to modify the template as indicated in this doc section: https://loopback.io/doc/en/lb4/Repositories.html

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Great work, @marioestradarosa!

I have reviewed the commits b70ea7e to 07780ba and found few places to discuss, see my comments below.

I did not review the integration tests in details and did not run the CLI command again to verify the issues I have reported were fixed, I'll trust you on this :)


### Notes

The optional argument `[<name>]`, referes to a valid datasource already created
Copy link
Member

Choose a reason for hiding this comment

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

I find it confusing that names is referring to datasource in lb4 repository command. I would expect the name control the name of the repository we are going to create. Since the repository name is build from the model name, maybe this argument can be omitted entirely?

Alternatively, can we use [model] argument for the model name?

Either way, I think the datasource should be specified via --dataSource argument?

Last but not least, can we move this "Notes" section after we describe the command line format first (line 29 currently)?

@raymondfeng @strongloop/sq-lb-apex thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way, I think the datasource should be specified via --dataSource argument?

I agree.

I find it confusing that names is referring to datasource in lb4 repository command.

It is extending from ArtifactGenerator and because of the multiple selection of models and thus mulitple repository generation feature:

  1. I couldn't see the name as the repository name because it can generate multiple ones
  2. ArtifactGenerator still was showing the name in the help see lb4 controller --help for instance
  3. I agree that it should be datasource name and this can be discarded
  4. I will be placeing the option --dataSource instead

Copy link
Member

Choose a reason for hiding this comment

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

Maybe your generator should be inheriting from a different base generator class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably the option is BaseGenerator, I just followed up the other options initially. let me change it and try it.


```ts
{
"name": "validDataSourceName",
Copy link
Member

Choose a reason for hiding this comment

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

Can we use dataSource instead of name here? (See my comment above.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. see above comment.


`--id` : _(Optional)_ name of the property serving as **ID** in the selected
model. Keep in mind that CLI will still check and try to infer a valid ID from
the model file and give it priority over the one supplied here.
Copy link
Member

Choose a reason for hiding this comment

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

Keep in mind that CLI will still check and try to infer a valid ID from the model file and give it priority over the one supplied here.

While I understand the reasoning why it makes sense to prefer the auto-inferred id type over user-provided, I am little bit concerned that the user does not have the option to take full control and force a specific id type. Should we perhaps introduce two CLI options?

Alternative 1:

  • --id disables id-type-inference and tells the generator to always use the type provided via this argument.
  • --default-id is used as a fallback if we cannot infer id type.

Alternative 2:

  • --id is used if we if we cannot infer id type
  • --auto-idcontrols whether id should be infered, --auto-id=false disables auto-inference and always use the type provided via --id

Thoughts?

cc @raymondfeng @strongloop/sq-lb-apex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about adding a boolean switch --force , that could allow the user to generate without inspecting or validating a datasource, a model and the id name , plus , with this switch we can use the name for the repository name also ?

Copy link
Member

Choose a reason for hiding this comment

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

We already have a switch called --yes that allow prompts to be skipped if a prompt has default value or the corresponding anwser exists in options.

I am concerned that having both --force and --yes would be confusing to our users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 , agree. i din't see --yes ,but that make sense. however, the --yes skips the prompts and that should be treated internally in the Generator as skipping the checkings also right?.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe so.

const ERROR_NO_MODELS_FOUND = 'No models found in';
const ERROR_NO_MODEL_SELECTED = 'You did not select any model';
const ERROR_NO_MODEL_SELECTED = 'You did not select a valid model';
const ERROR_NO_DIRECTORY = "couldn't find the directory";
Copy link
Member

Choose a reason for hiding this comment

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

How about 'The directory was not found: '?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 ok

let modelFile = path.join(
this.artifactInfo.modelDir,
modelName.toLowerCase() + '.model.ts',
`${utils.kebabCase(modelName)}.model.ts`,
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicating the logic implemented in lb4 model generator. I am concerned that if we change/fix this aspect of lb4 model, then we will most likely forget to update lb4 repository too.

https://github.com/strongloop/loopback-next/blob/888ece68463b87cef8e3ad7f67248ada78996a29/packages/cli/generators/model/index.js#L193-L194

Could you please extract a shared function to be used in both generators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

import {<%= repositoryTypeClass %>}} from '@loopback/repository';
import {<%= repositoryTypeClass %>} from '@loopback/repository';
import {<%= modelName %>} from '../models';
import {<%= dataSourceImportName %>} from '../datasources';
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering why repository-kv-template is using dataSourceImportName while repository-crud-template uses generic juggler.DataSource.

Can we use the same approach in both templates? I personally prefer the one where we import the actual dataSource class provided by the application, because this class can provide additional methods on top of the built-in juggler.DataSource API.

Also we try to keep imports sorted by alphabet. I think ../datasources should go before ../models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe @raymondfeng can help us here 😄 . A generated KV repository using the generic juggler.DataSource didn't compile until I followed the template in the tutorial see #1659 (comment)

const REPOSITORY_APP_PATH = 'src/repositories';
const INDEX_FILE = path.join(SANDBOX_PATH, REPOSITORY_APP_PATH, 'index.ts');

const SANDBOX_FILES = [
Copy link
Member

Choose a reason for hiding this comment

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

Yes please 👍

@marioestradarosa
Copy link
Contributor Author

@bajtos PTAL

@marioestradarosa marioestradarosa force-pushed the feat-add-lb-repository branch 2 times, most recently from 8174990 to f9f239f Compare September 15, 2018 07:18
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I reviewed the two new commits e9651e1 and
f9f239f. The patch is becoming so large that's it's very difficult to review it in whole.

I think it will be best to land it and make any further improvements in follow-up pull requests as needed.

Having said that, I think we should give a chance to @raymondfeng and @virkt25 to review the new version and wait a day or two before going forward with git-history cleanup and landing.

* Returns the modelName in the directory file format for the model
* @param {string} modelName
*/
exports.toModelFileName = function(modelName) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps getModelFileName would be a better function name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was method called toClassName already in utils.js , so I wanted to keep the same line with toModelFileName. But I believe we can change here toModelFileName and then probably open another PR to change the current one toClassName .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* Returns the repositoryName in the directory file format for the repository
* @param {string} repositoryName
*/
exports.toRepositoryFileName = function(repositoryName) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps getRepositoryFileName would be a better function name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos , I thought about this name, however, I wanted to keep consistency with the other method toClassName in the same utils.js file. so we have toClassName, toRepositoryFileName etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case I am ok with either way.

typeof <%= modelName %>.prototype.<%= idType %>
> {
constructor(
@inject('datasources.<%= dataSourceName %>') protected datasource: juggler.DataSource,
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this consistent with repository-kv-template.ts.ejs please and use dataSourceImportName instead of juggler.DataSource?

Also dataSourceImportName does not sound as a correct description to me, how about dataSourceClassName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dataSourceClassName sounds perfect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}`,
);

if (availableDatasources.length === 0) {
Copy link
Contributor

@jannyHou jannyHou Sep 17, 2018

Choose a reason for hiding this comment

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

should the length be checked before assigning the dataSourceClassName to this.artifactInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Yes, it should. Change done, thanks @jannyHou

@marioestradarosa marioestradarosa force-pushed the feat-add-lb-repository branch 2 times, most recently from a5a06f9 to 2715923 Compare September 17, 2018 15:38
Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Awesome PR!! Played around with it and really enjoyed the UX and the benefits of having a repository scaffolded instead of copy/paste + edit.

The use of AST to infer the id property is also very neat and definitely paves the way for more awesome work that can be build using AST parsing by learning from your implementation.

Left some comments for some slight improvements -- most are just nitpicks with only one blocker (easy fix though).

One more thing I would like to see done apart from the comments would be a test to cover the multiple repository generation -- however as this may involve some effort I'd be ok if this was done in a follow up PR.

Once again, outstanding work @marioestradarosa 💯


### Synopsis

Adds a new [Repository or Multiple Repositories](Repositories.md) class to a
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be worded slightly differently:

Adds a new Repository class (or multiple backed by the same datasource) to a LoopBack application.


<tr>
<td><code>lb4 repository</code></td>
<td>Add a new repository or multiple repositories to a LoopBack 4 application</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on this instead:

Add new repositories for selected model(s) to a LoopBack 4 application

// Data for templates
this.artifactInfo.fileName = utils.kebabCase(this.artifactInfo.name);
this.artifactInfo.outFile = `${this.artifactInfo.fileName}.model.ts`;
this.artifactInfo.outFile = utils.getModelFileName(this.artifactInfo.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay for this simplification!

};

// literal strings with artifacts directory locations
exports.repositoriesDir = 'repositories';
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if utils is the best place for this or if we should have a file for these constants?

Not a blocker -- fine for now and if needed, lets deal with it in a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think this should be fixed in a separate PR and involve the other generators in order to standardized it.

constructor(
@inject('datasources.<%= dataSourceName %>') datasource: <%= dataSourceClassName %>,
) {
super(<%= modelName %>,datasource);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: need a space after the comma.

this.artifactInfo.className = utils.toClassName(
this.artifactInfo.modelName,
);
this.artifactInfo.outFile = utils.getRepositoryFileName(
Copy link
Contributor

Choose a reason for hiding this comment

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

Overwriting outFile each time a model from modelNameList is processed will result in only the last generated repository file to be exported.

To write multiple files to index.ts, the following must be done:

  • Create an empty property in _setupGenerator() on artifactInfo. this.artifactInfo.indexesToBeUpdated = []`.
  • After setting outFile here, add the following: this.artifactInfo.indexedToBeUpdated.push({dir: this.artifactInfo.outDir, file: this.artifactInfo.outFile});

For implementation of updating indexes see: https://github.com/strongloop/loopback-next/blob/master/packages/cli/lib/artifact-generator.js#L112-L127

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice 👍 . However, I had to change the artifact-generator.js in order to work. if you take a look at the else on line 141, would reset this.artifactInfo.indexesToBeUpdated = []; the array in our case. I will do another commit with the changes in a minute.

);
}

if (debug.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this if is not needed as there is nothing process heavy about outFile.

Copy link
Contributor

Choose a reason for hiding this comment

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

No harm to check.

),
);

if (debug.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this if is not needed as source is just a string.

path.join(this.artifactInfo.outDir, this.artifactInfo.outFile),
);

if (debug.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed due to the inspect 👍 💯

idType = answer.propertyName;
}
}
this.artifactInfo.idType = idType;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Can we rename this to idProp or idProperty -- While looking at the template I was very confused because idType to me implied this was a type .. such as string, number, etc. but IIUC it is the name of the id property. --- I'd say it only needs to be changed on artifactInfo and in the *.ejs templates.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to idProperty and also change the method named to getModelIdProperty.

@marioestradarosa
Copy link
Contributor Author

One more thing I would like to see done apart from the comments would be a test to cover the multiple repository generation -- however as this may involve some effort I'd be ok if this was done in a follow up PR.

I added a test for multiple repository generation and fixed the lib/ArtifactGenerator.js in order to support the indexesToBeUpdated feature. I checked and no other generator was using this feature, please check this file.

I also fixed the nitpicks.

@virkt25 PTAL

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

LGTM! 💯 🎉

if (!fs.existsSync(this.artifactInfo.datasourcesDir)) {
return this.exit(
new Error(
`${ERROR_NO_DIRECTORY} ${this.artifactInfo.datasourcesDir}.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better message could be No datasources found at ${this.artifactInfo.datasourcesDir}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it for both models and repositories. @raymondfeng PTAL

Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

Fantastic work! I tried the command step by step:

  1. Without datasources and models
  2. Add a datasource
  3. Add a model

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Great effort! Thank you again for adding the cli for repositories.

@virkt25
Copy link
Contributor

virkt25 commented Sep 18, 2018

@marioestradarosa Looks like we have all the signoffs to land this PR, congrats 🎉

To land this can you please rebase this PR against master as well as squash the 17 commits into 1 (or as few as needed) to keep the history clean. Then You can just go ahead and merge it :)

@marioestradarosa marioestradarosa merged commit 739676b into loopbackio:master Sep 19, 2018
@marioestradarosa marioestradarosa deleted the feat-add-lb-repository branch September 20, 2018 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants