Simple review
dfee opened this issue · 5 comments
Was hoping to see the use of transactions here and here, and where you'd define that boundary.
As you're using ts-morph for metadata, did you run into any problems? I figured you'd have to generate types or something. In my project I stuck with reflect-metadata.
Glad to see you throwing up a repo :D
Was hoping to see the use of transactions here and here, and where you'd define that boundary.
MikroORM handles transactions automatically. I do this by calling flush at the end of my mutations.
I also fork the entity manager, so when multiple requests hit the backend, their entity maps won't collide.
I also did not specifically use ts-morph. It's a package used within MikroOrm to discover your entities files. This way a schema can be generated based off of your entity definitions.
I recommend you to checkout the MikroOrm documation! Don't forget to specify the next version, as I'm using the RC for v4 in this repository.
Transactions
MikroORM handles transactions automatically
I mean kinda. in the sense that everything within a unit of work is batched with a flush call into a transaction. But when we talk about transaction management that's not really what we mean. Flushing != transacting. Flushing is where you notify the DB of changes, and transacting is where you commit those changes.
From the docs on transactions:
However, MikroORM also allows (and encourages) you to take over and control transaction demarcation yourself.
Metadata
I also did not specifically use ts-morph
Using ts-morph is implicit (it's the default strategy). Read more on the metadata-provider docs.
You should also be generating cache files when using ts-morph:
After the discovery process ends, all metadata will be cached. By default, FileCacheAdapter will be used to store the cache inside ./temp folder in JSON files.
You can generate production cache via CLI command mikro-orm cache:generate.
Versioning
You're also using an alpha version that's a bit old. Just beware! It seems like the first RC will hit this week though!
Using ts-morph is implicit (it's the default strategy). Read more on the metadata-provider docs.
It was default in v3, now it is not (as it brings the TS as a runtime dependency, so it had to be separated to its own package - @mikro-orm/reflection). So no metadata caching in place either. I just need to update the docs (I already did in some places, edit: actually you are referring to the stable docs, those are for v3, in v4 docs it's already correct).
You're also using an alpha version that's a bit old. Just beware! It seems like the first RC will hit this week though!
Not sure why you think so, he has rc.0 there:
https://github.com/driescroons/mikro-orm-graphql-example/blob/master/package.json#L18
One issue I see is the @mikro-orm/cli being used as regular dependency - that will bring some things including ts-morph itself (and therefore TS as well), as it is also used in EntityGenerator and MigrationGenerator. Better to have that only as dev dependency, if you care about the bundle size (P.S. - I don't, and do it this way too so I can access the CLI in production, mainly to run migrations - but that could be achieved programatically with a dependency just to @mikro-orm/migrations, basically on 2 lines of code).
Better to have that only as dev dependency
true
(P.S. - I don't, and do it this way too so I can access the CLI in production, mainly to run migrations - but that could be achieved programatically with a dependency just to @mikro-orm/migrations, basically on 2 lines of code).
This was the reason 😄 . I have some ideas in mind for potential features / refactoring. I'll add that to my todo!
Thanks for the review btw @dfee @devin-fee-ah (idk what your main account is 😄 )