Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): lb4 model to scaffold model files #1487

Merged
merged 3 commits into from
Jul 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions examples/todo/src/models/todo.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Entity, property, model} from '@loopback/repository';
import {Entity, model, property} from '@loopback/repository';

@model()
export class Todo extends Entity {
Expand All @@ -27,22 +27,18 @@ export class Todo extends Entity {
@property({
type: 'boolean',
})
isComplete: boolean;
isComplete?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to change this to optional? If we do change it, could you also find references to this model in the documentation and fix that up as well?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the decoration @property({type: 'boolean'}) means that the property is optional at least as far as the persistence layer is concerned (juggler & connectors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only reason is this is how the CLI generated file will look ... I wanted to keep it consistent for users looking at this vs. what they get when they follow the tutorial.

Docs (including tutorial) will be updated in a follow up PR.


@property({
type: 'string',
})
remindAtAddress: string; // address,city,zipcode
remindAtAddress?: string; // address,city,zipcode

// TODO(bajtos) Use LoopBack's GeoPoint type here
@property({
type: 'string',
})
remindAtGeo: string; // latitude,longitude

getId() {
Copy link
Member

Choose a reason for hiding this comment

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

What happened to this function - are you sure we don't need it any more?

Copy link
Member

Choose a reason for hiding this comment

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

Should we perhaps modify the template used by the CLI tool to always include getId() implementation, preferably after the constructor function?

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 think with the refactor my comment on GitHub got lost somewhere ... but the reason for me to not include the getId function is as follows:

  • The underlying Entity class actually implements it based on the metadata ... so any property can be the id field and it handles it correctly (and while we can do this with the CLI ... I don't see the point of overriding the logic from the base class.

Here's the reference code: https://github.com/strongloop/loopback-next/blob/08c2d896367a7f8ecc5153578f2a698eefdea7a1/packages/repository/src/model.ts#L218-L231

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I guess my concern is about performance - an explicit getId implementation returning this.id is surely faster than the generic solution processing model definition.

But as Donald Knuth said, premature optimization is the root of all evil. I am ok with your proposed change, we can always add explicit getId() implementation later, when there is a legitimate need.

return this.id;
}
remindAtGeo?: string; // latitude,longitude

constructor(data?: Partial<Todo>) {
super(data);
Expand Down
1 change: 1 addition & 0 deletions packages/cli/bin/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ function setupGenerators() {
path.join(__dirname, '../generators/datasource'),
PREFIX + 'datasource',
);
env.register(path.join(__dirname, '../generators/model'), PREFIX + 'model');
env.register(
path.join(__dirname, '../generators/example'),
PREFIX + 'example',
Expand Down
1 change: 1 addition & 0 deletions packages/cli/generators/controller/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
if (debug.enabled) {
debug(`Artifact output filename set to: ${this.artifactInfo.outFile}`);
}

// renames the file
let template = 'controller-template.ts.ejs';
switch (this.artifactInfo.controllerType) {
Expand Down
241 changes: 241 additions & 0 deletions packages/cli/generators/model/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
// Node module: @loopback/cli
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

'use strict';

const ArtifactGenerator = require('../../lib/artifact-generator');
const debug = require('../../lib/debug')('model-generator');
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto with debug, it's not used in this file; consider using it to log the answers we get back from the prompts perhaps or removing it altogether (none of the other generators have it). Also, same applies to the datasource generator index.js file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Added debug for property names and property info. debug is used in the datasource generator ...

Copy link
Contributor

Choose a reason for hiding this comment

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

oops :-), never mind in that case.

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

/**
* Model Generator
*
* Prompts for a Model name, Model Base Class (currently defaults to 'Entity').
* Creates the Model Class -- currently a one time process only.
*
* Will prompt for properties to add to the Model till a blank property name is
* entered. Will also ask if a property is required, the default value for the
* property, if it's the ID (unless one has been selected), etc.
*/
module.exports = class ModelGenerator extends ArtifactGenerator {
constructor(args, opts) {
super(args, opts);
}

_setupGenerator() {
this.artifactInfo = {
type: 'model',
rootDir: 'src',
};

this.artifactInfo.outDir = path.resolve(
this.artifactInfo.rootDir,
'models',
);

// Model Property Types
this.typeChoices = [
'string',
'number',
'boolean',
'object',
'array',
'date',
'buffer',
'geopoint',
'any',
];

this.artifactInfo.properties = {};
this.propCounter = 0;

return super._setupGenerator();
}

setOptions() {
return super.setOptions();
}

checkLoopBackProject() {
return super.checkLoopBackProject();
}

async promptArtifactName() {
await super.promptArtifactName();
this.artifactInfo.className = utils.toClassName(this.artifactInfo.name);
this.log(
`Let's add a property to ${chalk.yellow(this.artifactInfo.className)}`,
);
}

// Prompt for a property name
async promptPropertyName() {
this.log(`Enter an empty property name when done`);

delete this.propName;

const prompts = [
{
name: 'propName',
message: 'Enter the property name:',
validate: function(val) {
if (val) {
return utils.checkPropertyName(val);
} else {
return true;
}
},
},
];

const answers = await this.prompt(prompts);
debug(`propName => ${JSON.stringify(answers)}`);
if (answers.propName) {
this.artifactInfo.properties[answers.propName] = {};
this.propName = answers.propName;
}
return this._promptPropertyInfo();
}

// Internal Method. Called when a new property is entered.
// Prompts the user for more information about the property to be added.
async _promptPropertyInfo() {
if (!this.propName) {
return true;
} else {
const prompts = [
{
name: 'type',
message: 'Property type:',
type: 'list',
choices: this.typeChoices,
},
{
name: 'arrayType',
message: 'Type of array items:',
type: 'list',
choices: this.typeChoices.filter(choice => {
return choice !== 'array';
}),
when: answers => {
return answers.type === 'array';
},
},
{
name: 'id',
message: 'Is ID field?',
type: 'confirm',
default: false,
when: answers => {
return (
!this.idFieldSet &&
!['array', 'object', 'buffer'].includes(answers.type)
);
},
},
{
name: 'required',
message: 'Required?:',
type: 'confirm',
default: false,
},
{
name: 'default',
message: `Default value ${chalk.yellow('[leave blank for none]')}:`,
when: answers => {
return ![null, 'buffer', 'any'].includes(answers.type);
},
},
];

const answers = await this.prompt(prompts);
debug(`propertyInfo => ${JSON.stringify(answers)}`);
if (answers.default === '') {
delete answers.default;
}

Object.assign(this.artifactInfo.properties[this.propName], answers);
if (answers.id) {
this.idFieldSet = true;
}

this.log(
`Let's add another property to ${chalk.yellow(
this.artifactInfo.className,
)}`,
);
return this.promptPropertyName();
}
}

scaffold() {
if (this.shouldExit()) return false;

debug('scaffolding');

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

// Resolved Output Path
const tsPath = this.destinationPath(
this.artifactInfo.outDir,
this.artifactInfo.outFile,
);

const modelTemplatePath = this.templatePath('model.ts.ejs');

// Set up types for Templating
const TS_TYPES = ['string', 'number', 'object', 'boolean', 'any'];
const NON_TS_TYPES = ['geopoint', 'date'];
Object.entries(this.artifactInfo.properties).forEach(([key, val]) => {
// Default tsType is the type property
val.tsType = val.type;

// Override tsType based on certain type values
if (val.type === 'array') {
if (TS_TYPES.includes(val.arrayType)) {
val.tsType = `${val.arrayType}[]`;
} else if (val.type === 'buffer') {
val.tsType = `Buffer[]`;
} else {
val.tsType = `string[]`;
}
} else if (val.type === 'buffer') {
val.tsType = 'Buffer';
}

if (NON_TS_TYPES.includes(val.tsType)) {
val.tsType = 'string';
}

if (
val.defaultValue &&
NON_TS_TYPES.concat(['string', 'any']).includes(val.type)
) {
val.defaultValue = `'${val.defaultValue}'`;
}

// Convert Type to include '' for template
val.type = `'${val.type}'`;

if (!val.required) {
delete val.required;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? What I'm getting from this is if val.required does not exist, delete the key off of val. Is the code manually defining the key as undefined or null somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code is actually designed to delete val.required if required is set to false based on feedback from @jannyHou ... we don't need to decorate the property with required: false since that's the default if not present.

}

if (!val.id) {
delete val.id;
}
});

this.fs.copyTpl(modelTemplatePath, tsPath, this.artifactInfo);
}

async end() {
await super.end();
}
};
14 changes: 14 additions & 0 deletions packages/cli/generators/model/templates/model.ts.ejs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import {Entity, model, property} from '@loopback/repository';

@model()
export class <%= className %> extends Entity {
<% Object.entries(properties).forEach(([key, val]) => { %>
@property({<% Object.entries(val).forEach(([propKey, propVal]) => {%>
<%if (propKey !== 'tsType') {%><%= propKey %>: <%- propVal %>,<% } %><% }) %>
})
<%= key %><%if (!val.required) {%>?<% } %>: <%= val.tsType %>;
<% }) %>
constructor(data?: Partial<<%= className %>>) {
super(data);
}
}
73 changes: 52 additions & 21 deletions packages/cli/lib/artifact-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,27 +93,7 @@ module.exports = class ArtifactGenerator extends BaseGenerator {
});

if (generationStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, I think we should generationStatus file by file now instead of a global flag. For example,

models/model-1.ts (success)
models/model-2.ts (skipped)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be ok if I made a follow up task for this -- I assume this is needed for lb4 openapi which can call multiple generators under the hood?

/**
* Update the index.ts in this.artifactInfo.outDir. Creates it if it
* doesn't exist.
* this.artifactInfo.outFile is what is exported from the file.
*
* Both those properties must be present for this to happen. Optionally,
* this can be disabled even if the properties are present by setting:
* this.artifactInfo.disableIndexUpdate = true;
*/
if (
this.artifactInfo.outDir &&
this.artifactInfo.outFile &&
!this.artifactInfo.disableIndexUpdate
) {
await updateIndex(this.artifactInfo.outDir, this.artifactInfo.outFile);
// Output for users
this.log(
chalk.green(' update'),
`${this.artifactInfo.relPath}/index.ts`,
);
}
await this._updateIndexFiles();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should updateIndexFiles be a step before end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of end at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving it would mean also moving/duplication the generationStatus check.


// User Output
this.log();
Expand All @@ -128,4 +108,55 @@ module.exports = class ArtifactGenerator extends BaseGenerator {

await super.end();
}

/**
* Update the index.ts in this.artifactInfo.outDir. Creates it if it
* doesn't exist.
* this.artifactInfo.outFile is what is exported from the file.
*
* Both those properties must be present for this to happen. Optionally,
* this can be disabled even if the properties are present by setting:
* this.artifactInfo.disableIndexUpdate = true;
*
* Multiple indexes / files can be updated by providing an array of
* index update objects as follows:
* this.artifactInfo.indexesToBeUpdated = [{
* dir: 'directory in which to update index.ts',
* file: 'file to add to index.ts',
* }, {dir: '...', file: '...'}]
*/
async _updateIndexFiles() {
// Index Update Disabled
if (this.artifactInfo.disableIndexUpdate) return;

// No Array given for Index Update, Create default array
if (
!this.artifactInfo.indexesToBeUpdated &&
this.artifactInfo.outDir &&
this.artifactInfo.outFile
) {
this.artifactInfo.indexesToBeUpdated = [
{dir: this.artifactInfo.outDir, file: this.artifactInfo.outFile},
];
} else {
this.artifactInfo.indexesToBeUpdated = [];
}

for (const idx of this.artifactInfo.indexesToBeUpdated) {
await updateIndex(idx.dir, idx.file);
// Output for users
const updateDirRelPath = path.relative(
this.artifactInfo.relPath,
idx.dir,
);

const outPath = path.join(
this.artifactInfo.relPath,
updateDirRelPath,
'index.ts',
);

this.log(chalk.green(' update'), `${outPath}`);
}
}
};
Loading