feature(main): Basic mixin implementation #1

kjdelisle · 2017-11-20T15:56:55Z

This PR will hopefully demonstrate what I consider to be a better practice for adding any ORMs to loopback4.

This should also mean that the legacy-juggler, and any possible future "juggler"/in-house ORMs would follow a pattern similar to this, so that all ORMs receive the same treatment with respect to API compatibility.

connected to loopbackio/loopback-next#537

Ideally, we'll be able to take this approach and enhance it, or learn from it to make an even better pattern for ORM wireup in loopback4.

raymondfeng · 2017-11-20T19:49:55Z

src/typeorm-repository-mixin.ts

+      // rules in TypeScript.
+      const options = args[0] as ConnectionOptions;
+      // XXX(kjdelisle): Add logging in case this fails?
+      this.connection = this.connectionManager.create(options);


Is only one connection is allowed for the whole app?

If this module ends up becoming something we want to keep, we can make it more robust.

For me the big question is "do we want to mount all of our ORMs like this, or is there a better way?"

There are a few different ways to mount an ORM:

Just use a mixin to add sugar methods to app and you are on your own.

Define a bunch of unified Repository interfaces in LoopBack and have the ORM implements an adapter to fit in.

I took a quick dip in the typeorm source and it seems like they let the drivers handle things like application pooling on a case-by-case basis: https://github.com/typeorm/typeorm/blob/master/src/connection/Connection.ts#L149-L152

It might be that their ConnectionManager is for handling disparate connections between datasources, and that each Connection represents an active session with a datasource that may be a connection, connection pool, or even a facade for something else?

bajtos

The proposal looks reasonable to me. I am concerned about the amount of global per-application state we have.

See my comment below for global registry instances.
Right now, you allow the app to connect to a single database only. IMO this is too limiting - what if the app developer needs to connect to a legacy db2 database to access employee data and to a more recently added MySQL database to access customer relations data, so that they can provide a single REST API shielding the API clients from the complexity of company's systems of record? IIUC, this is handled by the concept of a datasource in legacy juggler, where each datasource represents a different database (or a backend service).

bajtos · 2017-11-21T13:18:29Z

src/typeorm-repository-mixin.ts

+     */
+    typeOrmRepository<S>(ctor: Constructor<S>): Binding {
+      const repo = this.connection.getRepository<S>(ctor);
+      return this.bind(`repositories.${ctor.name}`).to(repo);


IIUC, this creates a single Repository instance that's shared across all incoming requests. Shouldn't we create local per-request repository instances?

Out of curiosity, have you tried to add support for transactions? It makes me wonder how easy/difficult that would be. See https://github.com/typeorm/typeorm/blob/master/docs/transactions.md

bajtos · 2017-11-21T13:23:51Z

@kjdelisle is there any particular reason for keeping this work private, hidden from LoopBack Next community collaborators?

kjdelisle · 2017-11-21T16:40:12Z

Is there any particular reason for keeping this work private, hidden from LoopBack Next community collaborators?

Technically, I wanted to keep this module for myself as my own open source contribution, but I migrated it to the org to make it easier for the team to review. I'd like to take it back if I can. :)

@bajtos

I am planning on reworking the way connections are handled (using the ConnectionManager), but I'm currently reworking the tests to use Dockerode like we do in some of our other connectors and once I've got that working, I'll have a free hand to make other changes.

kjdelisle · 2017-11-21T21:44:26Z

I think I've got the Dockerode setup done, so I should be able to run tests in Jenkins now.

Going to work on refactoring the ConnectionManager now.

kjdelisle · 2017-11-22T22:35:46Z

IIUC, this creates a single Repository instance that's shared across all incoming requests. Shouldn't we create local per-request repository instances?

I've now refactored it to the point where you can define your own connection instances. In the acceptance test, I create two repositories on separate connections to demonstrate that it works.

I can't get this to work with a Provider because I need the constructor for the entity to generate the repository instance. This means that anyone who wants to have their Repository instances provided in this way would have to make a unique provider for each Repository type, make a unique binding on the application context for each unique provider and then inject their entity constructor for use there.

Out of curiosity, have you tried to add support for transactions? It makes me wonder how easy/difficult that would be. See https://github.com/typeorm/typeorm/blob/master/docs/transactions.md

Looking at the syntax for the TransactionRepository, it's interesting that they're performing their own decorator-based injection to resolve the repository instances. Assuming there's no conflict with that, I think it would be possible to do something with this concept to resolve repository instances under transactions, though I'd have to play around with it. I would prefer to build on that concept in a subsequent PR, though.

b-admike · 2017-11-23T15:03:31Z

README.md

+# This will pull and setup a mysql instance to run acceptance tests against.
+source setup.sh
+# This runs install and test one after the other.
+npm it


b-admike · 2017-11-23T15:08:53Z

src/typeorm-mixin.ts

+    constructor(...args: any[]) {
+      super(...args);
+      this.typeOrmConnectionManager = new TypeORMConnectionManager();
+      this.bind('typeorm.connections.manager').to(


Can we use this.bind('typeorm.connections.manager').toClass(TypeORMConnectionManager) here?

This is meant to be a single, pre-instantiated instance. There's no reason to do that since it would require me to re-inject it back into the Application constructor just to make use of it (which is silly).

Oh geez. Overlooked the line above. Makes sense!

b-admike · 2017-11-23T15:13:24Z

test/acceptance.test.ts

+    it('can create entities', async () => {
+      const customer = getCustomer();
+      const repo = (await app.get(
+        `repositories.${Customer.name}`,


repositories.${customer.name}

It's the class name, Customer. It won't work with your recommendation.

kjdelisle self-assigned this Nov 20, 2017

kjdelisle requested review from raymondfeng, bajtos, virkt25, jannyHou, b-admike, shimks and dhmlau November 20, 2017 18:52

kjdelisle mentioned this pull request Nov 20, 2017

refactor(core): Switch to public accessibility loopbackio/loopback-next#758

Closed

2 tasks

kjdelisle added team-apex review labels Nov 20, 2017

raymondfeng reviewed Nov 20, 2017

View reviewed changes

kjdelisle force-pushed the implement-mixin branch from b64b0fb to c5d04bb Compare November 20, 2017 21:18

bajtos reviewed Nov 21, 2017

View reviewed changes

kjdelisle force-pushed the implement-mixin branch from c5d04bb to 9d48df6 Compare November 21, 2017 21:42

kjdelisle force-pushed the implement-mixin branch from 9d48df6 to 885e6de Compare November 22, 2017 20:51

kjdelisle changed the title ~~WIP - implement mixin~~ feature(main): Basic mixin implementation Nov 22, 2017

b-admike reviewed Nov 23, 2017

View reviewed changes

feature(main): Basic mixin implementation

9696903

kjdelisle force-pushed the implement-mixin branch from 885e6de to 9696903 Compare November 23, 2017 17:59

b-admike approved these changes Nov 23, 2017

View reviewed changes

kjdelisle merged commit b6b0dba into master Nov 24, 2017

kjdelisle deleted the implement-mixin branch November 24, 2017 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(main): Basic mixin implementation #1

feature(main): Basic mixin implementation #1

kjdelisle commented Nov 20, 2017 •

edited

Loading

raymondfeng Nov 20, 2017

kjdelisle Nov 20, 2017 •

edited

Loading

raymondfeng Nov 20, 2017

kjdelisle Nov 20, 2017

bajtos left a comment

bajtos Nov 21, 2017

bajtos commented Nov 21, 2017

kjdelisle commented Nov 21, 2017

kjdelisle commented Nov 21, 2017

kjdelisle commented Nov 22, 2017

b-admike Nov 23, 2017

b-admike Nov 23, 2017

kjdelisle Nov 23, 2017

b-admike Nov 23, 2017

b-admike Nov 23, 2017

kjdelisle Nov 23, 2017

feature(main): Basic mixin implementation #1

feature(main): Basic mixin implementation #1

Conversation

kjdelisle commented Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

kjdelisle Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Nov 21, 2017

kjdelisle commented Nov 21, 2017

kjdelisle commented Nov 21, 2017

kjdelisle commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjdelisle commented Nov 20, 2017 •

edited

Loading

kjdelisle Nov 20, 2017 •

edited

Loading