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

MikroORM v5 #1623

Closed
48 tasks done
B4nan opened this issue Mar 30, 2021 · 120 comments
Closed
48 tasks done

MikroORM v5 #1623

B4nan opened this issue Mar 30, 2021 · 120 comments

Comments

@B4nan
Copy link
Member

B4nan commented Mar 30, 2021

Progress

Breaking changes

  • Require TS 4.1+ to allow using template literals (maybe even 4.2 it if makes sense)
  • Require Node v14+
  • Some methods are now strictly typed (em.create(), em.assign(), qb.insert(), qb.update(), e.toObject(), e.toJSON(), ...)
  • FindOptions.orderBy/fields/ are now strictly typed
  • Remove additional find/findOne/... parameters (populate/orderBy/limit/offset) in favour of FindOptions
  • Use options parameter in em.fork()
  • Remove @Repository decorator (use @Entity({ customRepository: () => CustomRepository }) instead)
  • Enable populateAfterFlush by default
  • migrations.pattern is removed in favour of migrations.glob (chore: upgrade umzug to 3.0 #2556)

https://mikro-orm.io/docs/next/upgrading-v4-to-v5/
mikro-orm/nestjs-realworld-example-app#36

@B4nan B4nan pinned this issue Mar 30, 2021
@JanJakes
Copy link

JanJakes commented Mar 30, 2021

@B4nan Awesome!

One point, maybe kind of related to schema diffing, could be improving naming strategy capabilities. If I'm not mistaken, there is currently no way of defining naming strategies for PKs, indexes, FKs, and others. (TypeORM for instance has a bit more powerful naming strategies.)

Another (maybe a little bit wild) idea is supporting Prisma schema – not sure if doable or reasonable in any way, but it is an interesting way of defining DB schema – it may be a feature that brings some traction, and it may allow using Prisma migrations for people who want it, etc.

One more thing on my mind – with the current CLI (and migrations) it is hard to use dynamic configuration provided by some kind of configuration services (for instance fetching a config service from DI and passing that as a config to migrations). You either end up duplicating the config data in mikro-orm.config.ts or using migrations programmatically in your own CLI commands. Maybe just simply if mikro-orm.config.ts could export a function returning a promise with the config, it would probably solve the issue.

And last idea (I hope, haha) is adding the possibility of locking migrations table exclusively when running migrations. It will prevent devs from ever starting multiple migration runs in parallel – this is a common mistake in Docker environments when devs use either k8s init containers or run the migration command on container start. I fix similar to the following one prevents the error (and forces all other containers to wait for their turn):

await this.orm.em.transactional(async (em) => {
  await em.execute('LOCK TABLE migrations IN EXCLUSIVE MODE');
  await this.orm
    .getMigrator<Migrator>()
    .up({ transaction: em.getTransactionContext() });
});

(This is for Postgres and of course this may not be doable for all drivers.)

Well, those are just a few ideas, let me know what you think :)
And thanks for your amazing work!

@B4nan
Copy link
Member Author

B4nan commented Mar 30, 2021

One point, maybe kind of related to schema diffing, could be improving naming strategy capabilities. If I'm not mistaken, there is currently no way of defining naming strategies for PKs, indexes, FKs, and others. (TypeORM for instance has a bit more powerful naming strategies.)

Sure, good idea. Currently it lives in the schema helper, so it is possible to override with a custom driver only.

Another (maybe a little bit wild) idea is supporting Prisma schema – not sure if doable or reasonable in any way, but it is an interesting way of defining DB schema – it may be a feature that brings some traction, and it may allow using Prisma migrations for people who want it, etc.

Sounds a bit out of scope, on the other hand entity definition is already abstracted so it could be just about having another metadata provider for this.

One more thing on my mind – with the current CLI (and migrations) it is hard to use dynamic configuration provided by some kind of configuration services (for instance fetching a config service from DI and passing that as a config to migrations). You either end up duplicating the config data in mikro-orm.config.ts or using migrations programmatically in your own CLI commands. Maybe just simply if mikro-orm.config.ts could export a function returning a promise with the config, it would probably solve the issue.

It is already possible to export a promise from the ORM config.

And last idea (I hope, haha) is adding the possibility of locking migrations table exclusively when running migrations. It will prevent devs from ever starting multiple migration runs in parallel – this is a common mistake in Docker environments when devs use either k8s init containers or run the migration command on container start. I fix similar to the following one prevents the error (and forces or other containers to wait for their turn):

Agreed, if this single query is enough for postgres, we can definitely do it at least for postgres. Would be great if users of other drivers could come up with similar solutions for those, I'd expect at least mysql to be capable of this too.

@JanJakes
Copy link

It is already possible to export a promise from the ORM config.

Hm, I thought I tried that, maybe not. OK, will give it a shot!

Agreed, if this single query is enough for postgres, we can definitely do it at least for postgres. Would be great if users of other drivers could come up with similar solutions for those, I'd expect at least mysql to be capable of this too.

Yep, this works for me, exclusive lock indeed makes any other queries to the table wait. MySQL doesn't support transactional DDL but I guess LOCK TABLES migrations WRITE could probably do the same trick (I can try it out when I have some time).

@B4nan
Copy link
Member Author

B4nan commented Mar 30, 2021

The config promise support was added in 4.5 so its very recent addition: #1495

Not sure if we need to support also returning async callback, as this just awaits the returned config...

@JanJakes
Copy link

The config promise support was added in 4.5 so its very recent addition: #1495

Not sure if we need to support also returning async callback, as this just awaits the returned config...

I see, perfect! No need for a callback, I think, this should cover pretty much any case and it's even simpler.

@TheNoim
Copy link

TheNoim commented Mar 30, 2021

This is my wish list for micro orm v5:

  • custom indexes (define the sql of an index). Use case: Improved fulltext search
  • generated columns in postgres / real virtual columns in mysql. Use case: again improved fulltext search. I made a pr for typeorm and I wish micro orm would support it, too.
  • typeorm -> micro orm migration support in some way

@rubiin
Copy link
Contributor

rubiin commented Apr 9, 2021

One of the things that I would like to add is a named migration file similar to typeorm. Currently afaik , the file names are generated implicitly

@merceyz
Copy link
Contributor

merceyz commented Apr 17, 2021

Since MikroORM now supports batched updates, inserts, and deletes it would be great if it would support batched selects (#266) as well, even if it was just for simple SELECT * FROM X WHERE PrimaryKey = Y queries. It probably wouldn't be that useful for REST APIs but would be really powerful for GraphQL APIs.

@yayvery
Copy link

yayvery commented Apr 18, 2021

Since MikroORM now supports batched updates, inserts, and deletes it would be great if it would support batched selects (#266) as well, even if it was just for simple SELECT * FROM X WHERE PrimaryKey = Y queries. It probably wouldn't be that useful for REST APIs but would be really powerful for GraphQL APIs.

This would be incredible. Right now I have 20 DataLoaders, being able to eliminate these would be awesome!

@ikdi
Copy link

ikdi commented Apr 19, 2021

As for me it would be great to have Oracle DB support.

@B4nan
Copy link
Member Author

B4nan commented Apr 24, 2021

custom indexes (define the sql of an index). Use case: Improved fulltext search

Implemented as @Index({ expression: 'alter table book add index custom_index_expr(title)' }).

typeorm -> micro orm migration support in some way

I said this many times. I never really used typeorm, so can't provide much info about this. Better ask someone else who does. Also as the internals are very different (typeorm is kind of a QB when compared to MikroORM, as there is no UoW/in-memory state management), automatic migrations would never be really performant. Best what I can offer is this commit where I migrated the typeorm version to MikroORM, but it is a bit dated.

generated columns in postgres / real virtual columns in mysql

Blocked by knex not supporting it. Probably possible to get around, but I am tired of hacking around how knex works. Maybe I will give it a try and remove it as a whole from schema diffing, as the code gets bloated because of this a lot, and I already had to implement better part of things myself anyway. But I'd still like to leverage knex for the new drivers to come before I remove it.

You could probably use @Formula() to have similar experience if the issue is about selecting dynamic things, but it is somehow limited.

One of the things that I would like to add is a named migration file similar to typeorm. Currently afaik, the file names are generated implicitly

The filenames are generated based on configurable callback, and if you adjust the pattern option, you can have the timestamps on the beginning, and use pretty much any file name after that. I don't see a need to change anything about how that works. We might add a recipe for this.

Since MikroORM now supports batched updates, inserts, and deletes it would be great if it would support batched selects (#266) as well, even if it was just for simple SELECT * FROM X WHERE PrimaryKey = Y queries. It probably wouldn't be that useful for REST APIs but would be really powerful for GraphQL APIs.

I was looking into this finally, and after some time playing with this, I finally see how it works. I must say that it feels funny now, all those "N+1 safety" claims, as the dataloader approach is valid only in very specific constraints (e.g. calling Promise.all() on the top level for methods that use the data loader directly). I can see how this is valid for GQL, but even there it feels funny to claim something about N+1 safety when its enough to load something in a for loop while awaiting it (for me the most common way to introduce N+1), and boom, we have the N+1 problem, dataloader or not. I can't imagine how it could be usefull for other than GQL apps...

Anyway, as mentioned, I do understand how this would be helpful for GQL users, so will try to make it work at least for the reference/collection loading. But not really a priority for me.

@rubiin
Copy link
Contributor

rubiin commented Apr 24, 2021

recipe for named migration file will be great

@merceyz
Copy link
Contributor

merceyz commented Apr 24, 2021

I can't imagine how it could be usefull for other than GQL apps...

It's actually really useful for batch processing of entities as well, where you fetch a batch of entities then based on values in each entity fetch different kinds of entities

@B4nan
Copy link
Member Author

B4nan commented Apr 24, 2021

Can you provide some code, or a test case ideally? Maybe I just wasn't able to make it work the way it should. All the examples I came up with felt much better written with populate hints so I haven't seen any real benefits when used manually (again, for GQL it's a different story). Only way I was able to make the dataloader to batch things was via Promise.all() for the first level, if you want to benefit on other levels, it would have to be again in a single tick, so it can't be from inside a list of promises, right?

@B4nan
Copy link
Member Author

B4nan commented Apr 25, 2021

Would be great to get some feedback on the new strict EntityData (so things like em.create() or qb.insert(), PR: #1718) and EntityDTO (serialization, e.g. toObject(), PR: #1719) types. Both are already merged, so you can play with the implementation via dev version (5.0.0-dev.41 or later). It could be indeed more strict (e.g. nullability in EntityData), but it would highly reduce DX, but maybe I am wrong, hence calling out for some feedback :]

I also wanted to make the string populate work with dot notation (e.g. populate: ['book.tags']), but it feels much more complex than it sounds, with not much benefit (as we already have type safe object notation), so will probably skip that. If there are some TS wizards in here, I would very much appreciate some help with this, but from what I understand, it would require things like explicit depth/recursion checks just to make it work with cycles, which itself feels like it adds a lot of implementation burden.

If you see more places we could make more strict, I am all ears.

Another thing I'd like to hear some feedback on is removal of (not only) boolean parameters in EM and driver API in favour of options objects. Currently most of the methods either have very long signature or support two (or more) signatures, so I'd like to keep only the options parameter. This means that following (and many more) would be gone:

-async find(entityName: EntityName<T>, where: FilterQuery<T>, populate?: P, orderBy?: QueryOrderMap, limit?: number, offset?: number): Promise<Loaded<T, P>[]>;
+async find(entityName: EntityName<T>, where: FilterQuery<T>, options?: FindOptions<T, P>): Promise<Loaded<T, P>[]>;

-async findOne(entityName: EntityName<T>, where: FilterQuery<T>, populate?: P, orderBy?: QueryOrderMap): Promise<Loaded<T, P> | null>;
+async findOne(entityName: EntityName<T>, where: FilterQuery<T>, options?: FindOneOptions<T, P>): Promise<Loaded<T, P> | null>;

-nativeInsertMany(entityName: string, data: EntityDictionary<T>[], ctx?: Transaction, processCollections?: boolean, convertCustomTypes?: boolean): Promise<QueryResult>;
+nativeInsertMany(entityName: string, data: EntityDictionary<T>[], options?: InsertManyOptions<T>): Promise<QueryResult>;

It would improve readability, allow adding new options, and might as well improve TS performance and error handling (I guess we all know those TS2769: No overload matches this call errors).

@JanJakes
Copy link

Seeing the improvements for migrations I'd have another one – the possibility to use pure .sql migration files. (Or is that already possible and I missed it?)

@bkstorm
Copy link

bkstorm commented May 5, 2021

I think view entities is an important feature.
There are many views in my database, so I have to use raw sql or knex to query them, it's convenient if they are complex queries.

@rubiin
Copy link
Contributor

rubiin commented May 5, 2021

I think view entities is an important feature.
There are many views in my database, so I have to use raw sql or knex to query them, it's convenient if they are complex queries.

viewEntity exists in typeorm. Is it the same thing that you want? https://typeorm.io/#/view-entities

@merceyz
Copy link
Contributor

merceyz commented May 5, 2021

Can you provide some code, or a test case ideally? Maybe I just wasn't able to make it work the way it should. All the examples I came up with felt much better written with populate hints so I haven't seen any real benefits when used manually (again, for GQL it's a different story). Only way I was able to make the dataloader to batch things was via Promise.all() for the first level, if you want to benefit on other levels, it would have to be again in a single tick, so it can't be from inside a list of promises, right?

@B4nan - #1623 (comment)

It does indeed require a Promise.all (or something like it) at some point so the promises can run at the same time (potentially nested as well), the main use case I have is stream processing of events from an external source.

This is probably the most generic example I could give but hopefully it shows what I mean

async function main() {
	for await (const batchOfData of someStreamOfData) {
		await Promise.all(
			batchOfData.map(async (data) => {
				switch (data.type) {
					case 'fooAction':
						{
							const entity = await em.findOne(Foo, data.id);
							entity.someField = data.value;
						}
						break;
					case 'heartbeat':
						{
							const status = await em.findOne(Status, data.id);
							status.lastActive = new Date();
							status.logs.add(new LogEntry());
						}
						break;
				}
			})
		);

		await em.flush();
		em.clear();
	}
}

@bkstorm
Copy link

bkstorm commented May 8, 2021

I think view entities is an important feature.

There are many views in my database, so I have to use raw sql or knex to query them, it's convenient if they are complex queries.

viewEntity exists in typeorm. Is it the same thing that you want? https://typeorm.io/#/view-entities

Yes, I'm migratting from type orm to mikro orm, and I realize mikro does not suoport views.

@chrisbrantley
Copy link

@B4nan I'd like to add some additional thoughts regarding migrations. My team has been working with MikroORM for quite some time now and we have consistently run into some "gotchas" with migrations that mostly stem from using the current database schema as the basis for comparison rather than the current migrations in the code base. Many of us came from Django which uses the existing migrations as the basis for comparison and that results in a much more predictable result when dealing with migrations.

The biggest problem on our team is when we are working on multiple branches that have different entities. Many times I'll switch to a branch to assist a team mate and make some changes to their entities. I generate a migration for these changes. But I have to do manual clean up on them because the migrations include removing tables for entities that are in another branch! This problem would not exist if the schema diff was against the migrations and not the current schema.

To work around this we have implemented a bit of a hack with a script that first applies all existing migrations to a completely empty database and then we create new migrations on that database. This is essentially using the current migrations to do the diff. It would be nice if this was either not necessary OR if this was a built-in option of the migration system.

Thanks!

@B4nan
Copy link
Member Author

B4nan commented May 15, 2021

We can't make diffs based on migrations, migrations do not bare metadata, only SQL queries, they would first need to be executed somewhere. Also you say "against migrations", but migrations do not necessarily have to be "complete", e.g. you could start using them on existing database. Might be also performance heavy if we do use actual database, if you reach certain number of existing migrations (and I'd guess it would be rather small number).

But maybe we could find a way in the middle. The new schema diffing in v5 already works based on comparing metadata, so we could serialize the whole database state into some migration meta file. There would be just one file, if not found, we would use the current db state. If we create new migration, the file would be updated. It would be versioned, so if you check out different branch, you'd have the state from that branch. Will try to hack something together, doing it this way might be actually quite easy.

@chrisbrantley
Copy link

we could serialize the whole database state into some migration meta file.

That sounds like a clever solution.

@B4nan
Copy link
Member Author

B4nan commented May 15, 2021

A bit harder than I expected, but looks like it's working (#1815). If you are brave enough, it will be available in 5.0.0-dev.129.

@MousMakes
Copy link

Would be a nice feature if there is a way to pass metadata from the save/insert/... method to a hook. In example the user who performed the operation.

@darkbasic
Copy link
Collaborator

darkbasic commented May 21, 2021

Anyway, as mentioned, I do understand how this would be helpful for GQL users, so will try to make it work at least for the reference/collection loading. But not really a priority for me.

I already have reference/collection loading working in v3, as well as find/findOne (even though I don't support all the operators, just a subset). I've just migrated my codebase from v3 to v4 and I'm in the process of forward porting batched selects as well, but since it's a lot of work I've decided to skip v4 and target v5 directly. I'm not sure if I will ever be able to target the whole set of operators, but maybe this the right time to try to get it merged. I'll update my batched selects to v5 while also updating the PR to get rid of gql altogether and make it easier to review.

@darkbasic
Copy link
Collaborator

I've rebased my dataloader to v5, if someone wants to have a look you can find it here: #364 (comment)

@dzcpy
Copy link

dzcpy commented Nov 15, 2021

@WumaCoder I ended up creating my own cache layer (using subscribers to update the cache). It would be a bit too complicated for MikroORM to auto-update the cache. Consider there might also be fields related to other tables etc. But if it's feasable, this would be a super powerful feature.

@B4nan
Copy link
Member Author

B4nan commented Nov 16, 2021

I am considering dropping support for node 12/13 in v5. Given the stable release will probably happen early next year and the EOL of node 12 is 2022-04-30, it feels like it does not matter much to keep supporting it. Similarly we might require TS 4.5 (we will definitely adopt it right after it gets released, the question is whether there will be actual BCs like they did in 4.1 with string literal types).

There is no particular need for this right now, but given how long the v5 development took, I'd rather be more aggressive now, so we don't fall into "not possible to upgrade dependency A because it requires node 14+ and we keep supporting node 12" (or similarly with TS, e.g. knex required TS 4.1 in latest versions, so 4.x could not use it and the upgrade will need to wait for v5).

Anybody still stuck on node <14? Or anybody that would see upgrading of TS as problematic? I remember there were people stuck on some particular version of TS due to angular not supporting newer ones, etc.

(btw I also made quite a lot of progress in last week or two, if you are using v5 already, please try the latest changes)

@darkbasic
Copy link
Collaborator

I'm all in for node 14+, but I'm currently stuck with TS 4.4 because of #1623 (comment)
Since I'm already facing another regression with 4.0+ I'd say go ahead with 4.5, which will be required anyway for proper ESM support. So many issues with TS :(

@B4nan
Copy link
Member Author

B4nan commented Nov 18, 2021

So TS 4.5 is finally out, latest master is updated and also used in the realworld project, everything works fine there.

https://github.com/mikro-orm/nestjs-realworld-example-app/tree/v5

edit: also tried to run it with TS 4.1 and everything looks fine

@klausXR
Copy link

klausXR commented Dec 13, 2021

Is there an ETA for this release as it appears that everything has been checked? Great work.

@B4nan
Copy link
Member Author

B4nan commented Dec 13, 2021

Yeah most of the things are ready, I'd like to ship RC before christmas, and stable probably in january if there are no major issues found.

Will be also shipping one more patch release to 4.x branch, with some of the bug fixes that were easily portable.

@boredland
Copy link
Contributor

I am using v5 on daily basis and am very happy so far!

@klausXR
Copy link

klausXR commented Dec 19, 2021

I am using v5 on daily basis and am very happy so far!

How did you install it?

@boredland
Copy link
Contributor

I am using v5 on daily basis and am very happy so far!

How did you install it?

just yarn add [pkgname]@next

@B4nan
Copy link
Member Author

B4nan commented Dec 20, 2021

Added two more breaking changes to v5:

populateAfterFlush is enabled by default

After flushing a new entity, all relations are marked as populated, just like if the entity was loaded from the db. This aligns the serialized output of e.toJSON() of a loaded entity and just-inserted one.

In v4 this behaviour was disabled by default, so even after the new entity was flushed, the serialized form contained only FKs for its relations. We can opt in to this old behaviour via populateAfterFlush: false.

migrations.pattern is removed in favour of migrations.glob

Migrations are using umzug under the hood, which is now upgraded to v3.0. With this version, the pattern configuration options is no longer available, and has been replaced with glob. This change is also reflected in MikroORM.

The default value for glob is !(*.d).{js,ts}, so both JS and TS files are matched (but not .d.ts files). You should usually not need to change this option as this default suits both development and production environments.

Also upgraded the realworld example migration to latest v5.

Note that in latest v5, to setup migrations for both dev and prod environments, see this https://mikro-orm.io/docs/next/migrations/#running-migrations-in-production

With this setup, it should be possible to use it in both environments, since v5 no longer cares about the migration files suffix.

@Lioness100
Copy link
Contributor

Lioness100 commented Jan 2, 2022

I'm so excited for the strictly typed em.create(), but a little sad that every property is still optional, which results in a lot of runtime errors for me, the clumsy person I am haha. (but I understand why it couldn't be implemented)

@rubiin
Copy link
Contributor

rubiin commented Jan 2, 2022

any eta on v5

@B4nan
Copy link
Member Author

B4nan commented Jan 11, 2022

Two more small BCs done via #2605

  • default paths to seeders folder is just seeders and it respects the basePath
  • refreshDatabase method was moved from SeedManager to SchemaGenerator

@B4nan
Copy link
Member Author

B4nan commented Jan 23, 2022

One final breaking change via #2660 before I ship the RC:

Population no longer infers the where condition by default

This applies only to SELECT_IN strategy, as JOINED strategy implies the inference.

Previously when we used populate hints in em.find() and similar methods, the
query for our entity would be analysed and parts of it extracted and used for
the population. Following example would find all authors that have books with
given IDs, and populate their books collection, again using this PK condition,
resulting in only such books being in those collections.

// this would end up with `Author.books` collections having only books of PK 1, 2, 3
const a = await em.find(Author, { books: [1, 2, 3] }, { populate: ['books'] });

Following this example, if we wanted to load all books, we would need a separate
em.populate() call:

const a = await em.find(Author, { books: [1, 2, 3] });
await em.populate(a, ['books']);

This behaviour changed and is now configurable both globally and locally, via
populateWhere option. Globally we can specify one of PopulateHint.ALL and
PopulateHint.INFER, the former being the default in v5, the latter being the
default behaviour in v4. Locally (via FindOptions) we can also specify custom
where condition that will be passed to em.populate() call.

// revert back to the old behaviour locally
const a = await em.find(Author, { books: [1, 2, 3] }, {
  populate: ['books'],
  populateWhere: PopulateHint.INFER,
});

@B4nan
Copy link
Member Author

B4nan commented Jan 23, 2022

So here we go, first RC version is officially out! Here is the full changelog:

https://github.com/mikro-orm/mikro-orm/blob/master/CHANGELOG.md#500-rc0-2022-01-23

@DASPRiD
Copy link
Contributor

DASPRiD commented Jan 26, 2022

@B4nan Awesome to see the RC being released! Unless there are any major issues being reported, how soon can we expect the final release to drop?

@B4nan
Copy link
Member Author

B4nan commented Jan 27, 2022

I just want to polish the docs a bit (mainly fix things that are no longer valid), and write the release article. So codewise I don't plan anything, only bug fixes, and maybe #2677 as it seems a bit breaking to me, so would be better to deal with it in 5.0.0.

But writing docs and articles is kinda the least entertaining part of this, so I tend to postpone it :D I hope it won't be more than a week or two.

@darkbasic
Copy link
Collaborator

I would definitely love to see it merged in 5.0.0.
Btw I've just noticed v5 is using AsyncLocalStorage <3
I was planning to use it in my library to access the context from the providers without having to rely on DI or passing props from the resolvers but I was a bit scared of the memory leaks, I guess they must be a thing of the past.

@pepakriz
Copy link
Contributor

Hi! I've tested v5-rc0 in our codebase at Qerko and everything works correctly - the pipeline is green. Currently, we use just basic mikro-orm functionality so I didn't expect any major issues. But I think it's good to let you know that everything looks good from our side.

@B4nan
Copy link
Member Author

B4nan commented Jan 29, 2022

I've been thinking about how to preserve optionality in the EntityData type - in other words how to make em.create() require required properties. The problem here are properties with default values - there is no way to infer this information via mapped types, so I though we could introduce another symbol where users would explicitly state the optional properties (those that are not optional for typescript, e.g. createdAt: Date = new Date() or anything defined with ! with db default). We can automatically make collections and functions as optional, as well as known PKs (e.g. id/_id/uuid). It won't work for 100% cases, but it should cover most of it (and for those where it won't work, we will end up with the current behaviour, marking them as optional, so still overall improvement).

Following entity would require only name in the em.create() method (without the OptionalProps it would also require type):

@Entity()
export class Publisher {

  // union of keys that are defined as required but are in fact optional
  [OptionalProps]?: 'type';

  @PrimaryKey()
  id!: number;

  @Property()
  name!: string;

  @OneToMany({ entity: () => Book, mappedBy: 'publisher' })
  books = new Collection<Book>(this);

  @ManyToMany({ entity: () => Test, eager: true })
  tests = new Collection<Test>(this);

  @Enum(() => PublisherType)
  type = PublisherType.LOCAL;

}

Do you think it would be worthy addition?

@B4nan
Copy link
Member Author

B4nan commented Jan 30, 2022

So the above is now implemented, as well as runtime checks for required properties when flushing new entities. Worth noting this will now effectively force mongo users to specify optionallity via nullable: true in the decorators (unless ts-morph is being used, which can infer optionality). Runtime checks can be disabled globally via valideteRequired: false. To disable compile time checks we could use something like [OptionalProps]?: keyof T; in a base entity.

Will ship this as RC.1 later today (already available in 5.0.0-dev.680).

@B4nan
Copy link
Member Author

B4nan commented Jan 30, 2022

RC.1 is out!

https://github.com/mikro-orm/mikro-orm/blob/master/CHANGELOG.md#500-rc1-2022-01-30

@pantajoe
Copy link

Regarding the new OptionalProps I have a small question: In some models I have computed getters, e.g.:

@Entity()
export class User {
  [OptionalProps]?: 'createdAt' | 'updatedAt';

  @PrimaryKey({ columnType: 'uuid', defaultRaw: 'gen_random_uuid()' })
  id: string = uuidv4();

  @Property({ defaultRaw: 'CURRENT_TIMESTAMP' })
  createdAt: Date = new Date();

  @Property({ defaultRaw: 'CURRENT_TIMESTAMP', onUpdate: () => new Date() })
  updatedAt: Date = new Date();
  
  @Property({ nullable: false })
  firstName: string;

  @Property({ nullable: false })
  lastName: string;
  
  get fullName(): string {
    return `${this.firstName} ${this.lastName}`;
  }
}

It totally makes sense to include createdAt and updatedAt as optional props via the symbol. However, I also have to add fullName to the OptionalProps symbol, or otherwise I get a typescript warning in my editor when calling em.create without a fullName prop. Is there any way to distinguish between normal properties and custom computed get properties, so we won't have to include them to the OptionalProps symbol?

If not, it's still a great feature! So thanks anyhow ❤️

@B4nan
Copy link
Member Author

B4nan commented Jan 31, 2022

I don't think so, a getter is still a property. Technically you could have a regular (and required) property defined with get/set too, it is not always virtual. We don't have any information from decorators on type level either.

Obviously if anybody knows a trick to achieve this, I am all ears :] All I can think of is kind of a prisma style generated interfaces, so we can actually work with the decorator metadata. I would very much like to allow that (optionally) at some point.

@B4nan
Copy link
Member Author

B4nan commented Feb 3, 2022

RC.2 is out!

https://github.com/mikro-orm/mikro-orm/blob/master/CHANGELOG.md#500-rc2-2022-02-03

@B4nan
Copy link
Member Author

B4nan commented Feb 6, 2022

RC.3 is out, with some last minute changes, namely validation improvements

If there won't be any major issue, I will probably ship the stable tomorrow.

https://github.com/mikro-orm/mikro-orm/blob/master/CHANGELOG.md#500-rc3-2022-02-05

@B4nan
Copy link
Member Author

B4nan commented Feb 6, 2022

Time has come! Say hello to MikroORM 5!

image

https://medium.com/@b4nan/mikro-orm-5-stricter-safer-smarter-b8412e84cca4

@B4nan B4nan closed this as completed Feb 6, 2022
@B4nan B4nan unpinned this issue Nov 18, 2022
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

No branches or pull requests