feat(core): refactor internal dependencies to support Yarn PnP

[merceyz]

What's the problem this PR addresses?

MikroORM is currently trying to use dependencies without declaring them which doesn't work under strict dependency environments (Yarn PnP) and makes us rely on hoisting which is really fragile.

Example:
Running yarn add @mikro-orm/sqlite knex@0.19.0 will produce a tree where @mikro-orm/sqlite won't get the same version of knex as @mikro-orm/knex

. -> knex@0.19.0
  -> @mikro-orm/sqlite
  -> @mikro-orm/knex -> knex@0.21.0

How did you fix it?

Add the undeclared dependencies and make sure the version of knex is required from @mikro-orm/knex

[merceyz]

Now that the dependencies are declared correctly I can't get the build to pass, most likely because the dependencies are circular. Would appreciate some input on this

[B4nan]

I don't follow what your problem is, this definitely nothing mergable - the whole purpose of the core package is that it does not depend on anything, what you did is to include packages that will transitively include their dependencies. That totally beats the purpose of the split into multiple packages.

[B4nan]

Running yarn add @mikro-orm/sqlite knex@0.19.0 will produce a tree where @mikro-orm/sqlite won't get the same version of knex as @mikro-orm/knex

knex should not be installed like this, it is already transitive dependency of @mikro-orm/knex (which is a dependency of @mikro-orm/sqlite). just like sqlite3 is already a dependency of @mikro-orm/sqlite so you install only @mikro-orm/sqlite, not knex, not sqlite3.

[B4nan]

you should literally install only @mikro-orm/sqlite, at least that was the intension. it already includes @mikro-orm/core and @mikro-orm/knex as well as sqlite3.

@darkbasic thoughts?

[darkbasic]

Running yarn add @mikro-orm/sqlite knex@0.19.0 will produce a tree where @mikro-orm/sqlite won't get the same version of knex as @mikro-orm/knex

knex should not be installed like this, it is already transitive dependency of @mikro-orm/knex (which is a dependency of @mikro-orm/sqlite). just like sqlite3 is already a dependency of @mikro-orm/sqlite so you install only @mikro-orm/sqlite, not knex, not sqlite3.

I had to manually install knex because I needed to create a custom type for postgis and I needed a handle on Knex. I couldn't simply import it because it would be undefined behaviour (I have a lint rule which forbids extraneous imports). I think that re-exporting Knex could work for such use cases.

[darkbasic]

@darkbasic thoughts?

Unfortunately I have no experience with yarn pnp because it doesn't play well with tsc, but I agree that you should always declare the dependencies that you use. For example if in the mariadb package you need to access @mikro-orm/core you will have to list it, even if you depend on @mikro-orm/mysql-base which already depends on it: otherwise you will rely on an undefined behaviour because there could be multiple versions racing to be hoisted. Lock files and yarn resolutions help a lot to fix those issues, but they are annoying nonetheless. I personally use an eslint rule to forbid extranous imports, so every time I try to import something which I didn't declare a direct dependency on I get an error.

[B4nan]

As I said, I am fine with reexporting knex, as it makes sense to me.

I also don't like the usage of Knex as a namespace (there is no real reason for that, all the usage is just about types, so nothing that would actually matter, right?).

The bottom line is, core package can't depend on anything. Knex is optional transitive dependency of @mikro-orm/knex, it should not be installed manually as it is not a peer dependency.

For example if in the mariadb package you need to access @mikro-orm/core you will have to list it, even if you depend on @mikro-orm/mysql-base which already depends on it: otherwise you will rely on an undefined behaviour because there could be multiple versions racing to be hoisted. Lock files and yarn resolutions help a lot to fix those issues, but they are annoying nonetheless. I personally use an eslint rule to forbid extranous imports, so every time I try to import something which I didn't declare a direct dependency on I get an error.

But that is the exact purpose of peer dependencies, if we want to ensure there is only one copy and not more. What you are proposing would make it actually worse for the regular users, or not? Every direct dependency would result in a new copy, even if the versions are matching, or not?

[B4nan]

As I said, the idea was to import only the driver package, rest should be transitive dependencies, nothing users should install themselves in the root project. Maybe that was wrong (although I am not very convinced), and we should require also the core package, and set that as a peer dependency everywhere, that is something for discussion.

But knex should not be installed like this, it was never meant to be peer dependency.

Would be great to see a repository demonstrating the problem first, before firing a PR like this (that basically reverts all the outcomes of #475, as with those changes the core package is suddenly directly dependent on knex, umzug, ts-morph and typescript).

[darkbasic]

But that is the exact purpose of peer dependencies, if we want to ensure there is only one copy and not more. What you are proposing would make it actually worse for the regular users, or not? Every direct dependency would result in a new copy, even if the versions are matching, or not?

I think it's up to the package manager to hoist it or not, but I wouldn't rely on it. If a dep is meant to be a peer dep I agree that it would be better to omit it, but I would still declare it as a dev dependency to avoid undefined behaviour in the development environment (won't matter once packaged though). If it's a peer dep there is no chance for ub because the user will be forced to manually install it so there will be no need to hoist it because it already is. Anyway as I said I'm no pnp expert.

[merceyz]

the whole purpose of the core package is that it does not depend on anything

The thing is it does, it just doesn't declare it

mikro-orm/packages/core/src/MikroORM.ts

Line 110 in 576e7fe

const SchemaGenerator = require('@mikro-orm/knex').SchemaGenerator;

mikro-orm/packages/core/src/MikroORM.ts

Line 119 in 576e7fe

const EntityGenerator = require('@mikro-orm/entity-generator').EntityGenerator;

mikro-orm/packages/core/src/MikroORM.ts

Line 128 in 576e7fe

const Migrator = require('@mikro-orm/migrations').Migrator;

what you did is to include packages that will transitively include their dependencies

There is no guarantee that you'll get access to the transitive dependencies, it is undefined behaviour and just happens to work.

knex should not be installed like this, it is already transitive dependency of @mikro-orm/knex (which is a dependency of @mikro-orm/sqlite). just like sqlite3 is already a dependency of @mikro-orm/sqlite so you install only @mikro-orm/sqlite, not knex, not sqlite3.

That is besides the point, I'll give a different example:
yarn add company-acl @mikro-orm/sqlite

. -> company-acl -> knex@0.19.0
  -> @mikro-orm/sqlite -> @mikro-orm/knex -> knex@0.21.0

Which version of knex @mikro-orm/sqlite gets access to here is undefined behaviour. This is fixed by re-exporting it from @mikro-orm/knex and using createRequire for the internal parts/patching

because it doesn't play well with tsc,

@darkbasic If there are issues please report them back to us (yarn) and we'll take a look, it works perfectly fine for us https://github.com/yarnpkg/berry

[B4nan]

During development, the @mikro-orm/* paths are translated to relative paths, we don't actually develop against those packages, so where is the undefined behaviour? we developed agains the local files in packages folder.

Btw I guess the logic behind adding new core dependencies was those lines?
https://github.com/mikro-orm/mikro-orm/blob/dev/packages/core/src/MikroORM.ts#L108

If one does not like this dynamic require, don't use the methods and go the other way (create schema generator directly and pass the orm instead of asking the orm to create it).

By this logic, we would have to include every driver in the core package, as they are all required dynamically:
https://github.com/mikro-orm/mikro-orm/blob/dev/packages/core/src/utils/Configuration.ts#L234

I hope you all understand that this is not an option.

If it's a peer dep there is no chance for ub because the user will be forced to manually install it so there will be no need to hoist it because it already is.

What I had in my mind is not to require user to install the peer deps like knex, they would still be transitive, but the child packages would state it is a peer dependency. So with the knex example, knex would be direct dependency of @mikro-orm/knex (as it already is), and a transitive peer dependency of all its dependent packages (like @mikro-orm/mysql-base or @mikro-orm/mariadb).

[merceyz]

If one does not like this dynamic require, don't use the methods and go the other way (create schema generator directly and pass the orm instead of asking the orm to create it).
By this logic, we would have to include every driver in the core package, as they are all required dynamically:

Didn't even spot that one, seems the solution to core is to require on behalf of the user using createRequire

[B4nan]

The code is LGTM, I feel like it now makes all sense to me, thanks for bearing with me @merceyz @arcanis :]

Now we just need to make the CI green, I can help with that if needed.

[merceyz]

The code is LGTM, I feel like it now makes all sense to me, thanks for bearing with me

Perfect, and no worries 👍

[B4nan]

Changed the title as I dont consider this as a fix (v4 is not yet released), hope it makes sense...

[B4nan]

Thanks again!

[merceyz]

No problem, thanks for this great ORM 👍
I've been using v3 in production at work for about three months now and it's been a pleasure to work with.

[darkbasic]

As I said, I am fine with reexporting knex, as it makes sense to me.
I also don't like the usage of Knex as a namespace (there is no real reason for that, all the usage is just about types, so nothing that would actually matter, right?).

@B4nan @mikro-orm/knex currently reexports Knex as import * as Knex from 'knex'; export { Knex }, which leads to the following error:

Maybe it could be changed to import Knex from 'knex'; export { Knex }?

[B4nan]

We could do export { Knex, knex } from 'knex'; in v5, where knex is up to date.

Btw I believe that WKT problem can be now solved without using knex raw, see https://mikro-orm.io/docs/custom-types/#advanced-example---pointtype-and-wkt

[alexojegu]

Currently I am using custom types, but previously I was using this:

import { Knex } from "@mikro-orm/knex";

const knex = Knex.default({ client: "mysql" });