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

CLI: Import LB3 model(s) into LB4 project [EXPERIMENTAL] #3688

Merged
merged 1 commit into from
Sep 27, 2019

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Sep 9, 2019

See #2480

This pull request is adding a new CLI command lb4 import-lb3-models:

  • The command requires a path to a LB3 application (either the top-level project directory or the server file).
  • After the app is loaded via require(), the user is presented with a list of models to import.
  • Important: built-in models like User and AccessToken are included in that list.
  • For each selected model, a corresponding src/models/{model-name}.ts file is created.

Known limitations (features intentionally left out of this initial pull request):

/cc @strongloop/loopback-next

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • 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

👉 Check out how to submit a PR 👈

@bajtos bajtos added this to the Sept 2019 milestone milestone Sep 9, 2019
@bajtos bajtos self-assigned this Sep 9, 2019
@bajtos bajtos force-pushed the feat/import-lb3-model branch 2 times, most recently from 477d523 to 483c18e Compare September 9, 2019 12:34
@bajtos
Copy link
Member Author

bajtos commented Sep 19, 2019

Based on feedback from @raymondfeng and the fact that many important parts of model definitions are contributed by runtime (e.g. PK type and strict mode is decided by the connector), I decided to rework lb4 import-model to load the entire LB3 application and obtain model definitions from runtime metadata.

Screenshot of the new version at work, using our examples/lb3-application/lb3app:

$ lb4 import-model lb3app/server/server.js

**WARNING WARNING WARNING**
This command is experimental and not feature-complete yet.
Watch the progress at https://github.com/strongloop/loopback-next/issues/2480

? Select models to import: CoffeeShop
Model CoffeeShop will be created in src/models/coffee-shop.model.ts

   create src/models/coffee-shop.model.ts
   update src/models/index.ts

Here is the content of generated src/models/coffee-shop.model.ts:

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

@model({
  settings: {
    strict: false,
    forceId: false,
    base: 'PersistedModel',
    replaceOnPUT: true,
    validateUpsert: true,
    idInjection: true
  }
})
export class CoffeeShop extends Entity {
  @property({
    type: 'number',
    id: 1,
    generated: true,
    updateOnly: false,
  })
  id?: number;

  @property({
    type: 'string',
    required: true,
  })
  name: string;

  @property({
    type: 'string',
  })
  city?: string;

  // Define well-known properties here

  // Indexer property to allow additional data
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  [prop: string]: any;

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

export interface CoffeeShopRelations {
  // describe navigational properties here
}

export type CoffeeShopWithRelations = CoffeeShop & CoffeeShopRelations;

@raymondfeng what do you think, is this closer to the user experience you are envisioning for lb4 import-model?

@strongloop/loopback-next WDYT?

@bajtos bajtos force-pushed the feat/import-lb3-model branch 2 times, most recently from 19ddd92 to d231ac9 Compare September 19, 2019 10:55
@raymondfeng
Copy link
Contributor

@bajtos Please investigate CI failures.

@bajtos bajtos force-pushed the feat/import-lb3-model branch 3 times, most recently from d0ac234 to f84e454 Compare September 24, 2019 13:09
@bajtos bajtos changed the title CLI: Import LB3 model into LB4 project CLI: Import LB3 model(s) into LB4 project [EXPERIMENTAL] Sep 24, 2019
@bajtos bajtos requested review from a team and raymondfeng September 24, 2019 13:12
@bajtos bajtos marked this pull request as ready for review September 24, 2019 13:13
@bajtos
Copy link
Member Author

bajtos commented Sep 24, 2019

@raymondfeng thank you for the initial approval. The pull request was not done yet. Today, I added many new tests and fixed handling of edge cases. Now I consider the pull request ready for final review & landing.

Does it still LGTY?

@bajtos bajtos force-pushed the feat/import-lb3-model branch from f84e454 to 42ec9ed Compare September 24, 2019 13:15
};

// List of built-in LB models to exclude from the prompt
const EXCLUDED_MODEL_NAMES = [
Copy link
Contributor

@agnes512 agnes512 Sep 24, 2019

Choose a reason for hiding this comment

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

I am a bit lost here. In the docs, it says

you can modify your LB3 model to inherit from Model,
PersistedModel or KeyValueModel, then import the model and finally update
the generated model

if a model extends from the list, can it be imported or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

if a model extends from the list, can it be imported or not?

TL;DR: These two aspects are orthogonal. If a model extends from Model, PersistedModel or KeyValueModel (all of them are in the list), then it can be imported. If it inherits from a different model class, then it cannot be imported yet.

We have two list of model names:

  1. List of model names to hide from the CLI prompt when asking which app models to import. This list is EXCLUDED_MODEL_NAMES plus any model name starting with AnonymousModel_.

    The purpose of this filter is to hide models that cannot be imported (e.g. Model and PersistedModel are built-in in both LB3 and LB4) or which does not make sense to import (e.g. Email).

    The filter does not take into account model inheritance.

    I suppose I could improve the filter to take into account model inheritance. The difficult part is how to explain the user why some models are included in the list and some are not. I would prefer to invest our time into implementing import of models inheriting from other app models instead.

  2. List of base models that we know how to handle at LB4 side, see https://github.com/strongloop/loopback-next/blob/42ec9ed2b972fec9ae25b974b65c268a6b2ed34b/packages/cli/generators/import-lb3-model/migrate-model.js#L120-L124

    I created this list by picking built-in LB3 models that have their direct counter-part in LB4.

I'll be opening a new GH issue to implement support for importing models that are inheriting from a model that's not built into LB4. There are two major use cases - import from an LB3 built-in model like User and import from an application-specific model. There are few difficult aspects to figure out, that's why I am leaving this feature out of scope of the initial version.

  • How to know where to import the base model from (what path to use in the import statement)?
  • How to ensure that the base LB3 model has been either already imported to the LB4 app or is part of the import being in progress?

Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM 👏, I just have one question

<tr>
<td><code>lb4 import-lb3-model</code></td>
<td>Import a LoopBack 3 model to a LoopBack 4 application</td>
<td><a href="Importing-LB3-models.html">Model generator</a></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Importer for LoopBack 3 models

@bajtos bajtos force-pushed the feat/import-lb3-model branch from 42ec9ed to f67b98c Compare September 26, 2019 07:00
@bajtos
Copy link
Member Author

bajtos commented Sep 26, 2019

@agnes512 @raymondfeng @hacksparrow thank you for the feedback, I pushed a new commit to address your comments.

Does the patch LGTY now?

@bajtos
Copy link
Member Author

bajtos commented Sep 26, 2019

One more commit 644d33b - I reworked formatting of "Known limitations" in the docs and added links to follow-up GitHub issues.

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

Successfully merging this pull request may close these issues.

4 participants