[WIP - DO NOT MERGE] feat: PoC for typeorm repository integration #763

raymondfeng · 2017-11-27T23:17:49Z

Description

Related issues

connect to <link_to_referenced_issue>

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style guide

kjdelisle · 2017-12-04T16:08:59Z

@raymondfeng Would we be able to see an acceptance test that covers off a more advanced use case involving filters and joins (INNER JOIN and LEFT JOIN)? I'm curious to see how the DSL would support nesting of filters with the query building.

EDIT: I just realized the schema in use for the acceptance tests is a one-table schema, which prevents validating these use cases.

bajtos · 2017-12-04T17:47:55Z

packages/repository-typeorm/test/acceptance/typeorm-repository.ts

+import {MysqlConnectionOptions} from 'typeorm/driver/mysql/MysqlConnectionOptions';
+
+import {Entity as Base} from '@loopback/repository';
+import {Entity, Column, PrimaryGeneratedColumn, Repository} from 'typeorm';


Interesting. So you are not leveraging decorators from @loopback/repository to define models. I think that makes @loopback/repository less useful - for example client SDK generators cannot rely on a single set of decorators describing model schemas. Not a big deal, just something to keep in mind and document. I think this also means that all juggler-related stuff should be moved out of @loopback/repository into a standalone package, e.g. @loopback/repository-juggler so that people wishing to use their own ORM don't have to install juggler dependency.

As we discussed online, if we want a single Repository instance for consumers (REST API, GraphQL API, etc.), then we need also a single model schema. To get that, I think we need to map our decorators to TypeORM decorators, or provide a method for obtaining LDL definition for a TypeORM model.

@bajtos IMO I would like to have a 100% typeorm model, therefore I would expect to decorate my models using typeorm decorators.

As a platform developer I would like to have a pure typeorm implementation, not being wrapped or decorated by loopback.

I just would expect loopback to help me load the connection and automatically register my models for me.

bajtos · 2017-12-04T17:52:11Z

packages/repository-typeorm/src/repositories/typeorm-repository.ts

+
+  async update(entity: DataObject<T>, options?: Options): Promise<boolean> {
+    await this.init();
+    await this.typeOrmRepo.updateById(entity.getId(), <DeepPartial<T>>entity);


IMO, we should not need to change the type of entity here, we should change the API signature or DataObject<T> interface to support DeepPartial<T>.

bajtos · 2017-12-04T17:53:08Z

packages/repository/src/legacy-juggler-bridge.ts

+    options?: Options,
+  ): Promise<AnyObject> {
+    throw new Error('Not implemented');
+  }


The addition of execute method makes a lot of sense to me, I think this should be added to loopback-next in a standalone PR, including tests etc.

bajtos · 2017-12-04T17:53:58Z

packages/repository/src/loopback-datasource-juggler.ts

    where?: Where;
    fields?: Fields;
-    order?: Order[];
+    order?: string[];


Why is this change needed? Should we land it independently of this patch?

bajtos · 2017-12-04T17:55:46Z

packages/repository/src/query.ts

 * Order by direction
 */
-export type Direction = 'ASC' | 'DESC';
+export type Direction = 'ASC' | 'DESC' | 1 | -1;


Is 1 and -1 compatible with our existing connectors? I think this is showing the problem with trying to implement a solution that supports all ORMs - juggler uses ASC | DESC, TypeORM uses 1 | -1, how are we going to design a type system that allows only the valid values, depending on what ORM is used under the hood?

IMO, we should support only ASC and DESC, and the TypeORM bridge should convert these values to whatever TypeORM uses.

+1 on not trying to get 1 solution that supports all ORMs, you will end with code really difficult to maintain and unnecessary footprint.

I still think TypeORM should be an extension component (not mixin, nor an integrated solution) and must not be coupled in any way to the juggler. I honestly won't build on top of the Juggler, nor the REST components, I expect to cleanly install TypeORM in LoopBack 4.

bajtos · 2017-12-04T17:57:03Z

packages/repository/test/unit/repository-mixin/repository-mixin.test.ts

  class AppWithRepoMixin extends RepositoryMixin(Application) {}

-  class NoteRepo {
+  class NoteRepo implements Repository<any> {


I think this should be Repository<Note>?

Or perhaps Repository<Entity>?

bajtos · 2017-12-08T08:58:45Z

@jonathan-casarrubias thank you for joining the discussion!

IMO I would like to have a 100% typeorm model, therefore I would expect to decorate my models using typeorm decorators.
As a platform developer I would like to have a pure typeorm implementation, not being wrapped or decorated by loopback.
I just would expect loopback to help me load the connection and automatically register my models for me.
I still think TypeORM should be an extension component (not mixin, nor an integrated solution) and must not be coupled in any way to the juggler. I honestly won't build on top of the Juggler, nor the REST components, I expect to cleanly install TypeORM in LoopBack 4.

We are actually thinking the same! See @kjdelisle's proof-of-concept showing an external component that allows people to use TypeORM in their LoopBack4 applications here: https://github.com/strongloop/loopback4-extension-typeorm

@raymondfeng @kjdelisle I think this pull request can be closed now, what do you think? There are some bits of this patch that are fixing issues in our existing repository/juggler-integration code, I think it make sense to eventually land them. Any volunteers to extract them into a new pull request?

jonathan-casarrubias · 2017-12-08T13:06:26Z

@bajtos I saw that repo, I even created this issue strongloop-archive/loopback4-extension-typeorm#4 that is how I landed in here.

I agree this should be an extension but as I explained before and also did in that ticket, I'm not keen on the mixin approach -is too basic-, look the issue and I describe the experience I would expect.

I would expect a more robust component that can discover models and register these automatically, like I'm doing it with the proto files in the gRPC module. Once we get repository instances I would expect the component to expose these through providers, so developers would use these repositories using DI.

My expectation would be the extension to be a black box that automatically executes the described above with a set of typed options.

Agan I'm able to help on this one if needed :)

raymondfeng · 2017-12-11T16:41:53Z

Closing it now as we now have consensus around LB4 repositories based on this PoC.

I also cherry-pick a few commits into #792.

raymondfeng · 2017-12-11T16:43:09Z

FYI: I might move it into my personal repo as an illustration of how to adapt other ORMs into LB repository based persistence.

jonathan-casarrubias · 2017-12-12T22:54:05Z

I actually think that is an excellent Idea, having that as an option sounds legitimate.

raymondfeng requested a review from bajtos as a code owner November 27, 2017 23:17

raymondfeng force-pushed the repository-typeorm branch 13 times, most recently from 3087849 to 5cef9a1 Compare December 1, 2017 19:47

bajtos reviewed Dec 4, 2017

View reviewed changes

raymondfeng and others added 3 commits December 6, 2017 19:03

feat(repository): Add builders for Filter and Where

980da72

feat(repository): Add execute method to Repository

26d2975

feat: PoC for typeorm repository integration

1c585f2

raymondfeng force-pushed the repository-typeorm branch from 5cef9a1 to 1c585f2 Compare December 7, 2017 03:03

raymondfeng closed this Dec 11, 2017

kjdelisle deleted the repository-typeorm branch December 14, 2017 19:20

This was referenced Mar 30, 2020

[RFC WIP] feat(typeorm): add a component for TypeORM integration #4794

Closed

Integration with TypeORM #4853

Closed

[WIP - DO NOT MERGE] feat: PoC for typeorm repository integration #763

[WIP - DO NOT MERGE] feat: PoC for typeorm repository integration #763

Uh oh!

Conversation

raymondfeng commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

kjdelisle commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-casarrubias Dec 7, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-casarrubias Dec 7, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Dec 8, 2017

Uh oh!

jonathan-casarrubias commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raymondfeng commented Dec 11, 2017

Uh oh!

raymondfeng commented Dec 11, 2017

Uh oh!

jonathan-casarrubias commented Dec 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

raymondfeng commented Nov 27, 2017 •

edited

Loading

kjdelisle commented Dec 4, 2017 •

edited

Loading

jonathan-casarrubias Dec 7, 2017 •

edited

Loading

jonathan-casarrubias Dec 7, 2017 •

edited

Loading

jonathan-casarrubias commented Dec 8, 2017 •

edited

Loading