Allow components to contribute models + migration guide for LB3 components #5477

bajtos · 2020-05-18T12:57:33Z

In the first commit, I am adding a new optional property models to Component interface, to allow extension developers to declaratively contribute model classes to be bound in the target context. (I am aware that #5432 added createModelClassBinding, I just realized it's different from the existing approach we are using for components contributing repositories.)

In the second commit, I am starting the migration guide for LB3 components contributing models. Because we haven't figured out yet what should be our recommended solution for LB4 components wishing to contribute Entites and Repositories, I opened a follow-up issue #5476 to figure it out.

This is the last part of Spike: How to migrate LB3 components #4099.

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 👈

hacksparrow · 2020-05-18T13:44:35Z

I just realized it's different from the existing approach we are using for components contributing repositories.

Which existing approach? Link to example implementation?

packages/repository/src/mixins/repository.mixin.ts

raymondfeng · 2020-05-18T16:29:38Z

packages/repository/src/mixins/repository.mixin.ts

+          ? this.getComponentInstance<{
+              repositories?: Class<Repository<Model>>[];
+            }>(component)
+          : component;


This is not needed if 1cf38b1#r426750582 is addressed.

I am afraid the link does not point to any comment :(

I believe this is still needed to support the following use case:

app.mountComponentRepositories(SomeComponentClass);

(Note that mountComponentRepositories is a public method.)

packages/repository/src/mixins/repository.mixin.ts

raymondfeng

Please simplify the code by addressing 1cf38b1#r426750582

bajtos · 2020-05-19T06:55:09Z

I just realized it's different from the existing approach we are using for components contributing repositories.
Which existing approach? Link to example implementation?

The existing approach for repositories: components provide repositories property containing a list of repository classes they want to bind to the target application. Here is an example implementation from our test suite:

https://github.com/strongloop/loopback-next/blob/c9f7f25d80eca530f59b938e96fa020dd1dc9c09/packages/repository/src/__tests__/unit/mixins/repository.mixin.unit.ts#L225-L227

bajtos · 2020-05-19T08:19:54Z

Rebased on top of the latest master and addressed the review comments in f255ef8 ~~068eee7~~

@raymondfeng LGTY now?

bajtos · 2020-05-19T08:34:46Z

packages/repository/src/mixins/repository.mixin.ts

+
+      // `Readonly<Application>` is a hack to remove protected members
+      // and thus allow `this` to be passed as a value for `ctx`
+      function resolveComponentInstance(ctx: Readonly<Application>) {


I cannot use a private method in mixins :(

packages/repository/src/mixins/repository.mixin.ts:42:17 - error TS4094: Property '_resolveComponentInstance' of exported class expression may not be private or protected. 42 export function RepositoryMixin<T extends MixinTarget<Application>>( ~~~~~~~~~~~~~~~

docs/site/migration/components/models.md

raymondfeng · 2020-05-19T15:42:39Z

packages/repository/src/mixins/repository.mixin.ts

+   * An optional list of Model classes to bind for dependency injection
+   * via `app.model()` API.
+   */
+  models?: Class<Model>[];


Do we want to add dataSources?

I am not sure. DataSources typically require environment-specific configuration (e.g. database connection string), I am no sure if it makes sense to export them from a component. Either way, such change is out of scope of this pull request - feel free to open a follow-up GH issue to discuss this idea more.

docs/site/migration/components/models.md

bajtos · 2020-05-21T08:55:26Z

I have addressed the remaining suggestions on afda58d.

@raymondfeng @jannyHou @hacksparrow LGTY now?

jannyHou

👍 Good content, LGTM

Add a new optional property `models` to `Component` interface, to allow extension developers to declaratively contribute model classes to be bound in the target context. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

- Create a first version of the migration guide describing the simplest use case where the model is not persisted. - Add a link to a follow-up issue where we will discuss advanced scenarios. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos added Docs Repository Issues related to @loopback/repository package Migration labels May 18, 2020

bajtos requested a review from raymondfeng May 18, 2020 12:57

bajtos requested review from agnes512, deepakrkris and hacksparrow as code owners May 18, 2020 12:57

bajtos self-assigned this May 18, 2020

bajtos mentioned this pull request May 18, 2020

Spike: How to migrate LB3 components #4099

Closed

11 tasks

raymondfeng reviewed May 18, 2020

View reviewed changes

packages/repository/src/mixins/repository.mixin.ts Outdated Show resolved Hide resolved

raymondfeng reviewed May 18, 2020

View reviewed changes

packages/repository/src/mixins/repository.mixin.ts Outdated Show resolved Hide resolved

raymondfeng requested changes May 18, 2020

View reviewed changes

bajtos force-pushed the spike/component-migration--models-repositories branch from c9f7f25 to 068eee7 Compare May 19, 2020 08:19

bajtos requested a review from raymondfeng May 19, 2020 08:22

bajtos added the feature label May 19, 2020

bajtos commented May 19, 2020

View reviewed changes

bajtos force-pushed the spike/component-migration--models-repositories branch from 068eee7 to f255ef8 Compare May 19, 2020 08:38

jannyHou reviewed May 19, 2020

View reviewed changes

docs/site/migration/components/models.md Outdated Show resolved Hide resolved

raymondfeng reviewed May 19, 2020

View reviewed changes

hacksparrow reviewed May 20, 2020

View reviewed changes

docs/site/migration/components/models.md Outdated Show resolved Hide resolved

bajtos force-pushed the spike/component-migration--models-repositories branch from f255ef8 to afda58d Compare May 21, 2020 08:54

bajtos requested review from hacksparrow, jannyHou and raymondfeng May 21, 2020 08:55

hacksparrow approved these changes May 21, 2020

View reviewed changes

jannyHou approved these changes May 21, 2020

View reviewed changes

raymondfeng approved these changes May 21, 2020

View reviewed changes

bajtos added 2 commits May 21, 2020 16:31

feat(repository): allow components to contribute models for DI

de2d68b

Add a new optional property `models` to `Component` interface, to allow extension developers to declaratively contribute model classes to be bound in the target context. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos force-pushed the spike/component-migration--models-repositories branch from afda58d to 917757d Compare May 21, 2020 14:32

bajtos merged commit 7741fbc into master May 21, 2020

bajtos deleted the spike/component-migration--models-repositories branch May 21, 2020 14:48

mschnee mentioned this pull request May 21, 2020

Fix: bootmixin should use super.component() to get binding key #5516

Merged

3 tasks

Allow components to contribute models + migration guide for LB3 components #5477

Allow components to contribute models + migration guide for LB3 components #5477

Uh oh!

Conversation

bajtos commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

hacksparrow commented May 18, 2020

Uh oh!

Uh oh!

raymondfeng May 18, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos May 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raymondfeng left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 19, 2020

Uh oh!

bajtos commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos May 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raymondfeng May 19, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos commented May 21, 2020

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bajtos commented May 18, 2020 •

edited

Loading

bajtos commented May 19, 2020 •

edited

Loading

bajtos May 21, 2020 •

edited

Loading