[DO NOT MERGE] Add core classes for data modeling #62

raymondfeng · 2017-02-20T16:11:06Z

cc @bajtos @raymondfeng @ritch @superkhau

WIP to add core classes for data modeling & persistence

superkhau · 2017-02-20T22:34:18Z

The concept is LGTM.

bajtos · 2017-02-21T13:15:25Z

packages/juggler/lib/mixin.ts

+/**
+ * A constructor
+ */
+export type Constructable = new (...args: any[]) => Object;


Constructor's don't return a generic Object, they return a specific class instance.

I am proposing to use the following definition from TypeScript's wiki:

type Constructor<T> = new(...args: any[]) => T;

I would personally prefer design patterns from TypeScript team over our own inventions, I believe they are less likely to contain subtle flaws and should be easier to understand for new people (TypeScript developers) discovering LoopBack code base.

I would personally prefer design patterns from TypeScript team over our own inventions

👍

I tried that but it gives me quite a bit trouble. One thing I cannot do is Constructor<?> which means a unknown type of constructor.

Did you try Constructor<any> or Constructor<Object>? I think the latter should be the same as what you have right now.

bajtos · 2017-02-21T13:45:10Z

mixin<BC extends Constructable>(Base: BC): BC;

My concern is that mixin<BC extends Constructable>(Base: BC): BC will not allow IDEs to auto-complete new members added by the mixin.

The example provided on TS wiki uses an anonymous class as the return type.

function Tagged<T extends Constructor<{}>>(Base: T) {
    return class extends Base {
        _tag: string;
        constructor(...args: any[]) {
            super(...args);
            this._tag = "";
        }
    }
}

This has the unfortunate effect that typescript compiler is reporting Return type of exported function has or is using private name '(Anonymous class)' error when trying to build a .d.ts file (see microsoft/TypeScript#6307 for a bit more of context). Fortunately, this can be worked around:

interface Tagged {
  _tag: string;
}

interface WithTag {
  new(...args: any[]): Tagged;
}

function Tagged<T extends Constructor<{}>>(Base: T): T & WithTag {
    return class extends Base implements Tagged {
        _tag: string;
        constructor(...args: any[]) {
            super(...args);
            this._tag = '';
        }
    };
}

const TaggedPoint = Tagged(Point);
// VSCode shows the following hints:
// [tslint] variable name must be in camelcase or uppercase (variable-name)
// const TaggedPoint: typeof Point & WithTag

Based on that, I think the MixinProvider and MixinFunc APIs should be defined like this:

export interface MixinProvider<Mixin> {
  mixin<BC extends Constructor<BC>>(Base: BC): BC & Mixin;
}

export interface MixinFunc<Mixin> {
  <BC extends Constructor<BC>(Base: BC): BC & Mixin;
}

// usage
const fn: MixinFunc<WithTag> = Tagged;

Thoughts?

bajtos · 2017-02-21T13:45:24Z

packages/juggler/lib/mixin.ts

+   * Mix in properties/methods into a base class
+   * @param Base The base class
+   */
+  mixin<BC extends Constructable>(Base: BC): BC;


See #62 (comment)

bajtos · 2017-02-21T13:48:54Z

packages/juggler/lib/model.ts

+ * See https://en.wikipedia.org/wiki/Domain-driven_design#Building_blocks
+ */
+
+export abstract class Model {


abstract class or interface - what are the arguments to choose one over another?

As I has shown in my spike #54, TypeScript's type system compares classes (constructors) by reference, while interfaces are compared by content (member signatures).

What if we defined ModelMixin that would add these three new members? That would allow us to convert any TypeScript/JavaScript class (e.g. coming from a 3rd party package) into a model.

Thoughts?

bajtos · 2017-02-21T13:50:24Z

packages/juggler/lib/model.ts

+/**
+ * Base class for entities which have unique ids
+ */
+export abstract class Entity {


Is it desirable to have a class that's both a Model and an Entity at the same time? How are we going to achieve that, considering that JavaScript does not support multi-inheritance?

Entity should extend from Model

bajtos · 2017-02-21T13:54:03Z

@raymondfeng Good start! I reviewed the code, I find it difficult to understand the design in all details without seeing it in action. I think we really need to add a simple connector implementation and few unit tests to verify that all pieces fit together as envisioned. I am almost sure there are places that will need some tweaking to make them work.

bajtos · 2017-02-21T14:00:01Z

There is one design decision that may or may not be subjective.

In your proposal, the responsibility of converting a rich model instance into a plain data object lies in the connector - connector methods are receiving the original model instance.
In my spike [DO NOT MERGE] Spike - multiple copies of typedefs #54, I implemented conversion from model instance into plain data objects in the Repository class.

I don't know which choice is the best, but I think we should make careful consideration befor picking one.

What I see as benefits of converting model instances into data objects in Repository:

No need to repeat this step in every connector implementation. This may also prevent possible bugs where a community-maintained connector does not perform the conversion correctly (e.g. calls for (var prop in modelInstance) to iterate over all properties, instead of calling toObject).
I believe the current Connector API contract expects the connectors to receive a plain data object. If we preserve this, then I think it would be very cheap for loopback-next to support all existing connectors. The less work we have to make, the sooner we can release a usable loopback-next version.

ritch · 2017-02-22T15:55:13Z

packages/juggler/examples/models/address.ts

@@ -0,0 +1,8 @@
+import {ValueObject} from "../../lib/model";


Nit:

We should be consistent in import... eg:

import {ValueObject} from "juggler" // or in a real app "@loopback/juggler"

ritch · 2017-02-22T15:56:52Z

packages/juggler/examples/models/order.ts

+@model()
+class Order extends Entity {
+  @property({name: 'qty', mysql: {
+    column: 'QTY'


so clean... 👍

ritch · 2017-02-22T15:57:32Z

packages/juggler/examples/models/order.ts

+  id: string;
+  customerId: string;
+
+  @belongsTo()


is there some prior art for this? this doesn't really read right to me...

Yes. https://github.com/typeorm/typeorm#creating-a-one-to-one-relation or JPA.

ritch · 2017-02-22T16:00:27Z

packages/juggler/examples/models/order.ts

+import {property, model, belongsTo} from "../../lib/decorator";
+import {Customer} from './customer'
+
+@model()


If there isn't an argument to the decorator - can't this be @model

@model() can optionally accept an object as the argument to spell out more metadata.

ritch · 2017-02-22T16:01:47Z

packages/juggler/lib/dao.ts

+
+import {MixinBuilder, Constructable} from './mixin';
+
+module.exports = function (ds: DataSource, Model: Constructable) {


is this just for compatability with juggler? I find this code below incredibly confusing and obscure...

I'm not even sure where this is used...? Is there example somewhere?

I'll add more comments in the code.

ritch · 2017-02-22T16:08:45Z

@raymondfeng can you start by creating a real world or close to real world example where you aren't the one defining the requirements? Eg. take a real set of data models that aren't defined by you and create an example of how you would create them in LB-N?

That way we can really make sure that our design is going the right way.

superkhau · 2017-02-28T17:13:08Z

Closing for now, open again when the time comes.

raymondfeng · 2017-03-01T22:26:59Z

The purpose of this PR is to facilitate the on-going reviews/feedbacks. Let's keep it open but not merge it into master yet.

raymondfeng · 2017-03-01T22:44:13Z

I'll add more tests to show case the full usage.

ritch · 2017-03-03T06:55:44Z

I don't think its a matter of adding more tests - we need real world examples to validate our design

ritch · 2017-04-19T16:14:15Z

Hey @raymondfeng - can you re-open this when it comes back up on your plate? I want to keep the PR list actionable - so we can use it as a sort of TODO list for review / etc.

bajtos reviewed Feb 21, 2017

View reviewed changes

ritch suggested changes Feb 22, 2017

View reviewed changes

raymondfeng force-pushed the feature/persistence branch from ccc8786 to 360cf01 Compare February 22, 2017 21:50

superkhau closed this Feb 28, 2017

raymondfeng reopened this Mar 1, 2017

raymondfeng force-pushed the feature/persistence branch from 3b8ef67 to 0ca8bce Compare March 1, 2017 22:30

raymondfeng force-pushed the feature/persistence branch 6 times, most recently from 2d7102a to b1e9e62 Compare March 14, 2017 20:06

Raymond Feng added 5 commits April 5, 2017 14:46

Add core classes for data modeling

fc529a9

Add some model examples

60749a0

Add relations

d7fc416

Add tests for mixins

21ebdc3

Enhance mixins

da8310c

raymondfeng force-pushed the feature/persistence branch from 0df68a1 to d396ba6 Compare April 5, 2017 21:47

Raymond Feng added 8 commits April 5, 2017 14:47

Upgrade deps

90eb6d2

Add more comments

48e34bc

Update persistence

b23ba17

Tidy up typings

705a00d

Add typing tests

b5f6764

Further enhance core constructs

d396ba6

Add more samples

9d13a10

Enable decorator for repository

8a3e298

ritch closed this Apr 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] Add core classes for data modeling #62

[DO NOT MERGE] Add core classes for data modeling #62

raymondfeng commented Feb 20, 2017 •

edited by superkhau

Loading

superkhau commented Feb 20, 2017

bajtos Feb 21, 2017

ritch Feb 22, 2017

raymondfeng Mar 1, 2017

bajtos Mar 2, 2017

bajtos commented Feb 21, 2017

bajtos Feb 21, 2017

bajtos Feb 21, 2017

bajtos Feb 21, 2017

raymondfeng Mar 1, 2017

bajtos commented Feb 21, 2017

bajtos commented Feb 21, 2017

ritch Feb 22, 2017

ritch Feb 22, 2017

ritch Feb 22, 2017

raymondfeng Mar 1, 2017

ritch Feb 22, 2017

raymondfeng Mar 1, 2017

ritch Feb 22, 2017

raymondfeng Mar 1, 2017

ritch commented Feb 22, 2017

superkhau commented Feb 28, 2017

raymondfeng commented Mar 1, 2017

raymondfeng commented Mar 1, 2017

ritch commented Mar 3, 2017

ritch commented Apr 19, 2017


		import {MixinBuilder, Constructable} from './mixin';

		module.exports = function (ds: DataSource, Model: Constructable) {

[DO NOT MERGE] Add core classes for data modeling #62

[DO NOT MERGE] Add core classes for data modeling #62

Conversation

raymondfeng commented Feb 20, 2017 • edited by superkhau Loading

superkhau commented Feb 20, 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

bajtos commented Feb 21, 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

bajtos commented Feb 21, 2017

bajtos commented Feb 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritch commented Feb 22, 2017

superkhau commented Feb 28, 2017

raymondfeng commented Mar 1, 2017

raymondfeng commented Mar 1, 2017

ritch commented Mar 3, 2017

ritch commented Apr 19, 2017

raymondfeng commented Feb 20, 2017 •

edited by superkhau

Loading