Skip to content

fix(cli): change model template to properly render array types#1564

Merged
shimks merged 1 commit intomasterfrom
cli-model-template-building
Aug 8, 2018
Merged

fix(cli): change model template to properly render array types#1564
shimks merged 1 commit intomasterfrom
cli-model-template-building

Conversation

@shimks
Copy link
Contributor

@shimks shimks commented Jul 26, 2018 •
edited
Loading

The CLI used to build the templates without the quotation marks around value for arrayType.
Also removes the ugly new lines generated through the template

The generated code before:

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

@model()
export class Model extends Entity {
  
  @property({
    type: 'array',
    arrayType: string,
    
  })
  prop?: string[];
  
  @property({
    type: 'number',
    
  })
  propNumberTwo?: number;
  
  constructor(data?: Partial<Model>) {
    super(data);
  }
}

The generated code with this PR:

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

@model()
export class Model extends Entity {
  @property({
    type: 'array',
    arrayType: 'string',
  })
  prop?: string[];

  @property({
    type: 'number',
  })
  propNumberTwo?: number;

  constructor(data?: Partial<Model>) {
    super(data);
  }
}

fixes #1562

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

jannyHou
jannyHou reviewed Jul 27, 2018
View reviewed changes
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.

@shimks Good to have the fix for array type. Could you add a test case to verify the new generated property def?

@jannyHou
Copy link
Contributor

jannyHou commented Jul 27, 2018

Oh wait...I misunderstood the original issue.

The template decorates array property as

@Property({
type: 'array',
arrayType: object,

})
fields?: object[];

While I think the correct signature should be

@Property.array({
itemType: Object
})
fields?: object[];

By using @Property.array(), the itemType field has to be a Type instead of a string, see code

@bajtos
Copy link
Member

bajtos commented Jul 31, 2018

@shimks Could you please post two code snippets showing what was generated before and what code is generated after your change?

@shimks
Copy link
Contributor Author

shimks commented Aug 1, 2018

updated the main post with code example

@shimks shimks force-pushed the cli-model-template-building branch from 27ffe64 to 03d8fc3 Compare August 1, 2018 17:10
dhmlau
dhmlau reviewed Aug 1, 2018
View reviewed changes
packages/cli/generators/model/index.js

// Convert Type to include '' for template
val.type = `'${val.type}'`;
if (val.arrayType) {
Copy link
Member

@dhmlau dhmlau Aug 1, 2018

Choose a reason for hiding this comment

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

do you mean to include this change in this PR because this PR is only supposed for model template change?

Copy link
Contributor Author

@shimks shimks Aug 1, 2018

Choose a reason for hiding this comment

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

Nope, the PR's main purpose is to introduce quotation marks around val.arrayType. The formatting on the template is a bonus (imo)

bajtos
bajtos approved these changes Aug 2, 2018
View reviewed changes
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.

👏

@raymondfeng
Copy link
Contributor

raymondfeng commented Aug 2, 2018

arrayType is vague. Can we switch to something like itemType?

@shimks
Copy link
Contributor Author

shimks commented Aug 2, 2018

@raymondfeng I think that discussion should be held over at #1570, where we may need to tweak it along with deciding how juggler property definition would look like.

@shimks shimks force-pushed the cli-model-template-building branch from 03d8fc3 to e200c6d Compare August 7, 2018 15:50
@shimks shimks force-pushed the cli-model-template-building branch from e200c6d to 926d9d1 Compare August 8, 2018 15:56
@shimks shimks merged commit 2d43a61 into master Aug 8, 2018
@shimks shimks deleted the cli-model-template-building branch August 8, 2018 17:11
@bajtos bajtos mentioned this pull request Aug 8, 2018
Closed
31 tasks
@virkt25 virkt25 added this to the August Milestone milestone Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhmlau dhmlau dhmlau left review comments

@raymondfeng raymondfeng Awaiting requested review from raymondfeng

+3 more reviewers

@jannyHou jannyHou jannyHou left review comments

@bajtos bajtos bajtos approved these changes

@virkt25 virkt25 virkt25 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

August Milestone

Development

Successfully merging this pull request may close these issues.

compile error in generated model ts file when using lb4 model

6 participants

@shimks @jannyHou @bajtos @raymondfeng @virkt25 @dhmlau