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

[Model discovery] Generated code does not seem to be correct #2953

Closed
4 tasks
dhmlau opened this issue May 24, 2019 · 11 comments
Closed
4 tasks

[Model discovery] Generated code does not seem to be correct #2953

dhmlau opened this issue May 24, 2019 · 11 comments

Comments

@dhmlau
Copy link
Member

dhmlau commented May 24, 2019

Description / Steps to reproduce / Feature proposal

Using MySQL as the datasource, I created a table called Customer. Command to create the table is as below:

create table customer (custid VARCHAR(100), name VARCHAR(100));
ALTER table customer add primary key(custid);

The generated customer model looks like:

   @model([object Object])
   export class Customer extends Entity {
     @property({
       type: String,
       required: true,
       length: 100,
       id: 1,
       mysql: {"columnName":"custid","dataType":"varchar","dataLength":100,"dataPrecision":null,"dataScale":null,"nullable":"N"},
     })
     custid: String;

     @property({
       type: String,
       required: false,
       length: 100,
       mysql: {"columnName":"name","dataType":"varchar","dataLength":100,"dataPrecision":null,"dataScale":null,"nullable":"Y"},
     })
     name?: String;
     ...
   }

There are a few issues in the code:

  1. Model decorator is not correct: @model([object Object])
  2. For the string type, should it be string instead of String?
  3. Inside the property decorator, should the type be type: 'string', instead of type:String

Acceptance Criteria

Fix the generated code (@bajtos had some pointers for each of them, see #2953 (comment)):

  • Model decorator is not correct: @model([object Object])
  • For string type, it should be string instead of String
  • Inside the property decorator, it should be type: 'string', instead of type:String
  • For connector specific options, it should be represented as a regular TypeScript object, i.e. for
mysql: {"columnName":"name","dataType":"varchar","dataLength":100,"dataPrecision":null,"dataScale":null,"nullable":"Y"},
     })

it should be:

  @property({
    // ...
    mysql: {
      columnName: 'name',
      dataType: 'varchar',
      dataLength:100,
      dataPrecision:null,
      dataScale:null,
      nullable: 'Y'
    },
  })
  name?: String;
}```
@dhmlau
Copy link
Member Author

dhmlau commented May 24, 2019

cc @marvinirwin

@bajtos
Copy link
Member

bajtos commented May 24, 2019

Model decorator is not correct: @model([object Object])

Are we perhaps calling .toString() instead of converting the object to JSON or TypeScript? Let's use the same approach I introduced for lb4 model in #2843.

For the string type, should it be string instead of String?

Yes please. While String works too, it's considered as a bad practice in TypeScript.

@model([object Object])
export class Customer extends Entity {
  @property({
    // ...
  })
 custid: string;

  // ...
}

Inside the property decorator, should the type be type: 'string', instead of type:String

Juggler supports both forms. For consistency with the rest of LB4, it would be better to use type: 'string'.

I also noticed that connector-specific options are represented as JSON:

{ 
  @property({
    // ...
    mysql: {"columnName":"name","dataType":"varchar","dataLength":100,"dataPrecision":null,"dataScale":null,"nullable":"Y"},
     })
     name?: String;
}

It would be nice to represent them as a regular TypeScript object, similarly to what I introduced in #2843 for model settings.

{ 
  @property({
    // ...
    mysql: {
      columnName: 'name',
      dataType: 'varchar',
      dataLength:100,
      dataPrecision:null,
      dataScale:null,
      nullable: 'Y'
    },
  })
  name?: String;
}

@redaikidoka
Copy link

I'd like to add on that the @model([object Object])
does not compile, generating:
ERROR TS1005

It can be corrected by outputing @model() instead.

@gonadarian
Copy link

Sorry for jumping in as I'm a complete novice to Loopback, but...

I generated a model (can't remember how 😞) a few days ago (don't know the version as I updated to the latest just now 😞) which had no compile errors with this code @model({settings: {}}). Now I'm also getting @model([object Object]).

Looks like a regression, but can't be sure...

@dhmlau
Copy link
Member Author

dhmlau commented May 29, 2019

@gonadarian, thanks for the info. It's good to know. @bajtos has a suggestion on how to fix @model([object Object]). If you're interested in submitting a patch, please feel free!

Are we perhaps calling .toString() instead of converting the object to JSON or TypeScript? Let's use the same approach I introduced for lb4 model in #2843.

@marvinirwin
Copy link
Contributor

Sorry I'm late, If nobody else has started I'll try and create the fix PR in a day or two.

@dhmlau
Copy link
Member Author

dhmlau commented May 29, 2019

@marvinirwin, sure, that would be great.
I have another question for you.. Could you please take a look at my comment #2915 (comment)? I'm thinking that --schema option is needed, and we'll need to update the docs. Please let me know if it's not the case. Thanks!

@bajtos
Copy link
Member

bajtos commented May 31, 2019

Now I'm also getting @model([object Object]).

That look super weird to me! In #2843, I changed the code emitting @model to always stringify the model options into JavaScript code. It's entirely possible that I missed an edge case that does not work now, how can I reproduce the problem you are experiencing?

When I run lb4 app and lb4 model, then I see the following file being scaffolded in src/models/product.model.ts

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

@model({settings: {}})
export class Product extends Entity {

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

@bajtos
Copy link
Member

bajtos commented May 31, 2019

I think this issue will be fixed by #3015.

@samarpanB
Copy link
Contributor

I can confirm that this issue is fixed with latest cli version. Just tried with 1.16.1. But there is a different issue coming up on discovery. I'll raise that separately.

@bajtos
Copy link
Member

bajtos commented Jun 7, 2019

Thank you @samarpanB for the confirmation. I am closing this issue as fixed then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants