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

feat(mapping): allow defining schema via EntitySchema helper #335

Merged
merged 1 commit into from Feb 19, 2020

Conversation

B4nan
Copy link
Member

@B4nan B4nan commented Feb 17, 2020

One can now define entity schema via EntitySchema helper instead of using decorators or JavaScriptMetadataProvider.
Defining the EntitySchema is enough, blank entity class implementation will be created automatically. In that case,
user has to create new entity instances via em.create().

Closes #283

@B4nan
Copy link
Member Author

B4nan commented Feb 17, 2020

@whocaresk
Copy link
Contributor

whocaresk commented Feb 17, 2020

Lgfm, but i'll prefer pojo definition based on interface, as it could help with right prop definition via type inferring.
As a second thought, if we could infer interface from such definition (in example), then we'll never need to define it, and potentially cut off some work for end user.

@lookfirst
Copy link
Contributor

Missing nullable: true?

Agreed, inference ftw. =)

@whocaresk
Copy link
Contributor

An example from my current project, that uses typeorm:
https://gist.github.com/whocaresk/b012f20e4566e8edbeebd84c6f3fd012
The props interface is telling infra layer what shape of data does it expect, and after applying this interface to EntitySchema class - it wouldn't let define schema without required keys

@B4nan
Copy link
Member Author

B4nan commented Feb 17, 2020

Lgfm, but i'll prefer pojo definition based on interface, as it could help with right prop definition via type ineferring.

There is EntitySchema.create(meta) where you can pass an object with everything, if that is what you mean. Both ways should be typesafe. Also, the pojo way is already there, that is what JavaScriptMetadataProvider is for: https://mikro-orm.io/docs/usage-with-js/ (bad naming, but keeping that because of BC for now)

As a second idea, if we could infer interface from such definition (in example), then we'll never need to define it, and potentially cut off some work for end user.

Not sure if that is possible, you mean something like io-ts? Sounds like a chiken-egg problem - I think we can't have the definition typesafe if we do not design the interfaces manually. Or how would it work? e.g. when you define Book schema, you need Author schema already defined and vice versa.

Missing nullable: true?

It has persist: false, it is not stored in the db at all. It originates from the sqlite entities, will convert that to proper version field later (and then it will have default db value instead of being nullable).

The props interface is telling infra layer what shape of data does it expect, and after applying this interface to EntitySchema class - it wouldn't let define schema without required keys

I guess I understand now what you mean, it will force you to define all interface properties. Does that really work like this? Looking at your example, you have company property optional, but imagine it is required, how would it work for that case? Column is called companyId so I guess there is no type check for everything? Or does it work only for certain (scalar) property types? Will need to take a look at their implementation.

Btw the main reason for having dedicated methods to add properties is that there is some additional work done for default values (mainly when adding relations). It should also have the same type safety as using decorators - e.g. when adding a relation, name of the inversedBy/mappedBy field is type checked.

@whocaresk
Copy link
Contributor

whocaresk commented Feb 17, 2020

First of all, i'm from phone now, i'll get closer look tomorrow :)

Looking at your example, you have company property optional, but imagine it is required, how would it work for that case? Column is called companyId so I guess there is no type check for everything? Or does it work only for certain (scalar) property types? Will need to take a look at their implementation.

Implementation of typeorm are far (really far) from ideal, but it will force to define all the keys, required by interface, but only in column object. Usage of not defined columns in interface are provided by helper interface called DatabaseFields, it also strips out all non-scalar keys from interface. I've writed it by myself to overcome typeorm limitations. Btw, companyId shouldn't be there, it there just because i forgot to delete it :)

Not sure if that is possible, you mean something like io-ts? Sounds like a chiken-egg problem - I think we can't have the definition typesafe if we do not design the interfaces manually. Or how would it work? e.g. when you define Book schema, you need Author schema already defined and vice versa.

It more usable for more rapid development, and basic types are ok. I've seen something similar in redux-toolkit and custom Joi types, they are both pretty well (but not ideally) infer types from definitions. For relations, i think, we should pass extra arg, that would let infer type (already created schema, or class)

@B4nan
Copy link
Member Author

B4nan commented Feb 18, 2020

So after some time playing with type definitions, I managed to implement type safe way for this:

export const schema = new EntitySchema<Author4, BaseEntity5>({
  name: 'Author4',
  extends: 'BaseEntity5',
  properties: {
    name: { type: 'string' },
    email: { type: 'string', unique: true },
    age: { type: 'number', nullable: true },
    termsAccepted: { type: 'boolean', default: 0, onCreate: () => false },
    identities: { type: 'string[]', nullable: true },
    born: { type: DateType, nullable: true, length: 3 },
    bornTime: { type: TimeType, nullable: true, length: 3 },
    books: { reference: '1:m', type: 'Book4', mappedBy: 'author' },
    favouriteBook: { reference: 'm:1', type: 'Book4' },
    version: { type: 'number', persist: false },
  },
});

Pretty much everything is type checked in this approach:

  • all properties of the interface (excluding base entity interface if provided) are required to be part of properties object
  • each property definition needs to have one of type, customType or entity options
  • each entity either has reference option or is considered scalar
  • based on the value of reference option additional required props are enforced (via tagged union type)
  • mappedBy/inversedBy options are type checked for key name, the target type is inferred (also works for Collection<T>)

The OOP API is still there, used inside the constructor, so both approaches are valid, this being more strict. Going forward (v4), we might use EntitySchema instances internally instead of EntityMetadata objects, but that will be a BC.

Here is an example with custom entity class (for mongo):

export class BookTag implements MongoEntity<BookTag> {

  _id!: ObjectId;
  id!: string;
  name: string;
  books = new Collection<Book>(this);

  constructor(name: string) {
    this.name = name;
  }

}

export const schema = new EntitySchema<BookTag>({
  class: BookTag,
  properties: {
    _id: { type: 'ObjectId', primary: true },
    id: { type: 'string', serializedPrimaryKey: true },
    name: { type: 'string' },
    books: { reference: 'm:n', entity: () => Book, mappedBy: 'tags' },
  },
});

@B4nan B4nan force-pushed the entity-schema branch 6 times, most recently from 8aa069a to d34d30f Compare February 19, 2020 19:54
@mikro-orm mikro-orm deleted a comment from lgtm-com bot Feb 19, 2020
One can now define entity schema via EntitySchema helper instead of using decorators or JavaScriptMetadataProvider.
Defining the EntitySchema is enough, blank entity class implementation will be created automatically. In that case,
user has to create new entity instances via `em.create()`.

Closes #283
@B4nan B4nan merged commit 6d1a24c into master Feb 19, 2020
@B4nan B4nan deleted the entity-schema branch February 19, 2020 20:22
@B4nan
Copy link
Member Author

B4nan commented Feb 19, 2020

let's see how it works, available in 3.2

https://mikro-orm.io/docs/entity-schema/

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

Successfully merging this pull request may close these issues.

Different ways to describe schema
3 participants