Replace direct require() calls with an indirection function #3743

parisholley · 2022-11-15T13:34:41Z

Is your feature request related to a problem? Please describe.
Many new JS stacks use some type compiler (swc, esbuild, babel) and will often have some setting enabled that will attempt to in-line all code that is included via require(). That is problematic for this project since things like database drivers are only included if needed.

Describe alternatives you've considered
You can try to figure out how to exclude or mock the module name but for some projects (such as remix), the compiler stack isn't exposed to you unless you patch the project.

…with inlining

B4nan · 2022-11-15T13:41:36Z

I don't think we have require calls that cannot be already gotten around (e.g. for drivers you can pass the driver class via driver option instead of the type option). You can already use MikroORM with esbuild/swc, as long as you are fine with not using decorators for metadata.

Or is this about removing the require calls, so you don't need special handling for them (e.g. as in webpack)? We need defaults that work universally, so I don't think that's doable in near future.

…with inlining

B4nan · 2022-11-15T13:45:07Z

Looking at the PR, I believe that is the exact way to break things. This won't work in ESM project at all, if I'm not mistaken - will try that tomorrow, I have one ESM example project.

FYI we also have Utils.requireFrom and Utils.tryRequire, those are tested in various types of project.

parisholley · 2022-11-15T13:52:49Z

as long as you are fine with not using decorators for metadata

decorators work with swc, you just have to define type for every property and i have a registerMikro function for gathering classes in an array and passing to my mikro init logic

that is the exact way to break things

i'm not familiar enough with ESM to say

we also have Utils.requireFrom and Utils.tryRequire

alright, i can change if you think this isn't a breaking change

B4nan · 2022-11-17T13:17:13Z

Looks like it would be ok for ESM projects, but I am quite sure it would break webpack. This is the PR that changed it, you can see the previous state was pretty much the same as what your PR is doing.

cc @thekevinbrown, not sure you are still on board, would appreciate some webpack insight

parisholley · 2022-11-17T14:18:14Z

take this comment with a grain of salt as I am biased :) but using webpack to bundle server-side node_modules seems like a smell? i could get trying tree-shake dependencies or maybe having a single file to say, deploy to a serverless function provider, but even in SSR situations node_modules i find to be separate most of the time and a lot of projects meant for node aren't tree-shakable to begin with.

@B4nan on flip side, obviously a breaking change, could we not just have the user pass an instance/class/etc to config instead of driver "names" (same thing with migrations), and this problem goes away?

B4nan · 2022-11-17T14:24:39Z

Well, I don't have any opinion about this, never used webpack for it, so not interested in discussing it in any way (which is why I was tagging Kevin, he introduced the change, I am sure he will have some opinions). But your change is reverting a fix, that's what I care about - you might fix something for yourself, and break something that worked for others. If we are to do it, we need a working guide for webpack. I dont care if it will suggest doing X or Y, but it needs to be correct and working.

Note that you can already pass a driver class instead of using the type option. The option was added because that way you can set your driver via env vars, so we need some kind of dynamic require in place.

thekevinbrown · 2022-11-18T00:21:44Z

Hey @B4nan and @parisholley! I'm still around and we do still use Mikro.

We deploy our server side code to Lambda. It's really nice to have a single file which is the entire application and all of its dependencies at a particular version which we then just throw at a stock standard Lambda image with node. This makes it easy to deploy / roll back / etc. We use webpack currently (but are using ESBuild in dev already, so will likely be doing that in prod soon) to achieve this.

While there are other methods (like layers) which we could use to manage the application in more of an un-bundled way, this would increase complexity, and we've been in production like this for a couple of years at this point.

Most compilers / bundlers that can inline require() contents also expose options so you can control this behaviour, either by stubbing out modules that aren't found, or by simply rewriting the statement to null. Absent changes in Mikro that'd be my recommended approach so as not to break people who use webpack / ESBuild.

In general my preference would be to get Mikro out of the require() game here altogether by removing the ability to specify a name and just having the consumer of the library pass in an instance of whatever platform they want to the configuration. That way they can get it from wherever they want.

parisholley · 2022-11-18T00:55:47Z

@thekevinbrown yeah, figured lambda bundling was backstory here, i always use layers to avoid fighting bundle-unfriendly projects (and to get around lambda function size limitations) but it does add another layer to DevOps.

Most compilers / bundlers that can inline require() contents also expose options

suppose it depends on what world of JS you live in, SSR heavy frameworks don't expose very much and you'd have to dig into their code to patch their vite/esbuild/swc/etc pipelines. at the moment, i have to choose between hacking their framework or hacking mikro.

I agree that the only real solution here is to remove the requires out all together (in any capacity) and require the user to include them and that would work with any bundler system. as it stands, only webpack users in your situation are benefiting from the "fix", but this would be breaking change for v5.

options I see with v5:

add every require'd module to my package.json, probably not of much consequence
continue with patching those lines out (PITA with upgrades)
introduce a new main/index (eg: @mikro/core/v6) that uses an alternate version without requires and potentially make it default in next major

B4nan · 2022-11-18T10:31:27Z

We can surely get rid of the type option, at least to a certain degree - v5.5 added the driver-specific MikroORM exports that specify the driver type by class. As mentioned, the type was mainly there for those who want to use env vars only, but again, we have driver-specific defineConfig helpers that do the same, so we don't really need the type option if those new constructs are used.

There are more places that require things dynamically, but I guess that is mostly for the external stuff, that can be always used in other ways - we could keep something like Utils.tryRequire or Utils.requireFrom in there to preserve the API for people who use things "normally", if not, just use the package directly (e.g. import the entity generator from its package).

Totally agree that it would be best to get rid of it all. We will still need some dynamic require for entities, migrations, etc, but that's again all opt-in.

I don't think we should care much about this in v5, it does not seem to be possible to fix it in a non-breaking way, and I'd rather not come up with some alternative exports here (it would be really just a partial solution). I'd like to start the development rather soon, planning maybe one or two more feature releases, then we can branch 5.x and keep master for v6, backporting fixes to 5.x when appropriate, keeping all features to v6.

Note that if we get rid of the MikroORM class shortcuts for entity/schema generator/migrator/seeder, we will also remove the cycles from dependency graph, due to which things like turborepo don't work (and lerna v6 has the exact same ignorance when it comes to optional peer deps unfortunately).

B4nan · 2022-11-18T10:58:34Z

On the other hand, I must say I really like the new shortcuts (e.g. orm.schema)... Maybe optional dependencies would be more appropriate here? I used peer deps because as those will be installed by user, and I believe NPM installs optional deps by default, so it wouldn't be really a solution.

Or make it work the other way, users would "install extensions" in the ORM config, which would basically provide (safe) implementation for those methods. Every extension like that could export a register method:

import { defineConfig } from '@mikro-orm/postgres';
import { register as migrator } from '@mikro-orm/migrations';
import { register as entityGenerator } from '@mikro-orm/entity-generator';
import { register as seeder } from '@mikro-orm/seeder';

export default defineConfig({
  dbName: 'test',
  extensions: [migrator, entityGenerator, seeder],
});

Or maybe this way?

import { defineConfig } from '@mikro-orm/postgres';
import { Migrator } from '@mikro-orm/migrations';
import { EntityGenerator } from '@mikro-orm/entity-generator';
import { SeedManager } from '@mikro-orm/seeder';

export default defineConfig({
  dbName: 'test',
  extensions: [Migrator, EntityGenerator, SeedManager], // those would have a static `register` method
});

Also tagging @merceyz for some insights around yarn PNP, if we are to redesign this in v6, I'd want to be compatible with that too. But I believe we should be fine if we get rid of all the static require calls (and optional peer dependencies too I guess), right?

Register your extensions in the ORM config this way: ```ts import { defineConfig } from '@mikro-orm/postgres'; import { Migrator } from '@mikro-orm/migrations'; import { EntityGenerator } from '@mikro-orm/entity-generator'; import { SeedManager } from '@mikro-orm/seeder'; export default defineConfig({ dbName: 'test', extensions: [Migrator, EntityGenerator, SeedManager], // those would have a static `register` method }); ``` This will replace the static requires we use for extensions like entity generator or seeder. Related: #3743

B4nan · 2022-11-18T14:02:09Z

I think #3773 should do it. Once we start with v6 development, we can just remove all of the static requires as well as the type option and we should be good to go. I will also probably do the reexport of all core/knex classes in the driver package so users shouldn't need to use the core package in their imports at all, to reduce confusion.

It's actually quite tempting to start with the development rather early. I do have some things to release, but new features could go to v6 after that. Backporting fixes should be simpler than with v4 and v5.

Register your extensions in the ORM config this way: ```ts import { defineConfig } from '@mikro-orm/postgres'; import { Migrator } from '@mikro-orm/migrations'; import { EntityGenerator } from '@mikro-orm/entity-generator'; import { SeedManager } from '@mikro-orm/seeder'; export default defineConfig({ dbName: 'test', extensions: [Migrator, EntityGenerator, SeedManager], // those would have a static `register` method }); ``` This will replace the static requires we use for extensions like entity generator or seeder. Related: #3743

merceyz · 2022-11-18T16:36:15Z

Also tagging @merceyz for some insights around yarn PNP, if we are to redesign this in v6, I'd want to be compatible with that too. But I believe we should be fine if we get rid of all the static require calls (and optional peer dependencies too I guess), right?

Yeah, that seems fine 👍

It will be removed in v6 in favour of driver exported `defineConfig` and `MikroORM` class, or the `driver` option. Instead of specifying the `type` we now have several options: 1. use `defineConfig()` helper imported from the driver package to create your ORM config: ```ts title='mikro-orm.config.ts' import { defineConfig } from '@mikro-orm/mysql'; export default defineConfig({ ... }); ``` 2. use `MikroORM.init()` on class imported from the driver package: ```ts title='app.ts' import { MikroORM } from '@mikro-orm/mysql'; const orm = await MikroORM.init({ ... }); ``` 3. specify the `driver` option: ```ts title='mikro-orm.config.ts' import { MySqlDriver } from '@mikro-orm/mysql'; export default { driver: MySqlDriver, ... }; ``` Related #3743

B4nan · 2022-12-03T10:20:52Z

I hope #3814 should cover this. v6 branch is set up to publish v6 canary builds, so once it's merged, we can start testing how it actually behaves across the different frameworks and environments. I removed most of the peer dependencies that caused cycles, the type option (kept the MIKRO_ORM_TYPE env var for now, with a createRequire used under the hood).

There were some places where we did a static `require()` call, e.g. when loading the driver implementation based on the `type` option. Those places were problematic for bundlers like webpack, as well as new school build systems like vite. ### The `type` option is removed in favour of driver exports Instead of specifying the `type` we now have several options: 1. use `defineConfig()` helper imported from the driver package to create your ORM config: ```ts title='mikro-orm.config.ts' import { defineConfig } from '@mikro-orm/mysql'; export default defineConfig({ ... }); ``` 3. use `MikroORM.init()` on class imported from the driver package: ```ts title='app.ts' import { MikroORM } from '@mikro-orm/mysql'; const orm = await MikroORM.init({ ... }); ``` 5. specify the `driver` option: ```ts title='mikro-orm.config.ts' import { MySqlDriver } from '@mikro-orm/mysql'; export default { driver: MySqlDriver, ... }; ``` > The `MIKRO_ORM_TYPE` is still supported, but no longer does a static require of the driver class. Its usage is rather discouraged and it might be removed in future versions too. ### ORM extensions Similarly, we had to get rid of the `require()` calls for extensions like `Migrator`, `EntityGenerator` and `Seeder`. Those need to be registered as extensions in your ORM config. `SchemaGenerator` extension is registered automatically. > This is required only for the shortcuts available on `MikroORM` object, e.g. `orm.migrator.up()`, alternatively you can instantiate the `Migrator` yourself explicitly. ```ts title='mikro-orm.config.ts' import { defineConfig } from '@mikro-orm/mysql'; import { Migrator } from '@mikro-orm/migrations'; import { EntityGenerator } from '@mikro-orm/entity-generator'; import { SeedManager } from '@mikro-orm/seeder'; export default defineConfig({ dbName: 'test', extensions: [Migrator, EntityGenerator, SeedManager], // those would have a static `register` method }); ``` Closes #3743

B4nan · 2022-12-03T12:15:44Z

#3814 is merged to https://github.com/mikro-orm/mikro-orm/tree/v6 and published as 6.0.0-dev.6, so closing. Let's test it and open new issues if there are more changes needed.

There were some places where we did a static `require()` call, e.g. when loading the driver implementation based on the `type` option. Those places were problematic for bundlers like webpack, as well as new school build systems like vite. Instead of specifying the `type` we now have several options: 1. use `defineConfig()` helper imported from the driver package to create your ORM config: ```ts title='mikro-orm.config.ts' import { defineConfig } from '@mikro-orm/mysql'; export default defineConfig({ ... }); ``` 3. use `MikroORM.init()` on class imported from the driver package: ```ts title='app.ts' import { MikroORM } from '@mikro-orm/mysql'; const orm = await MikroORM.init({ ... }); ``` 5. specify the `driver` option: ```ts title='mikro-orm.config.ts' import { MySqlDriver } from '@mikro-orm/mysql'; export default { driver: MySqlDriver, ... }; ``` > The `MIKRO_ORM_TYPE` is still supported, but no longer does a static require of the driver class. Its usage is rather discouraged and it might be removed in future versions too. Similarly, we had to get rid of the `require()` calls for extensions like `Migrator`, `EntityGenerator` and `Seeder`. Those need to be registered as extensions in your ORM config. `SchemaGenerator` extension is registered automatically. > This is required only for the shortcuts available on `MikroORM` object, e.g. `orm.migrator.up()`, alternatively you can instantiate the `Migrator` yourself explicitly. ```ts title='mikro-orm.config.ts' import { defineConfig } from '@mikro-orm/mysql'; import { Migrator } from '@mikro-orm/migrations'; import { EntityGenerator } from '@mikro-orm/entity-generator'; import { SeedManager } from '@mikro-orm/seeder'; export default defineConfig({ dbName: 'test', extensions: [Migrator, EntityGenerator, SeedManager], // those would have a static `register` method }); ``` Closes #3743

parisholley added the enhancement New feature or request label Nov 15, 2022

parisholley added a commit to parisholley/mikro-orm that referenced this issue Nov 15, 2022

feat(mikro-orm#3743): replace direct require calls with util to help …

e633a83

…with inlining

parisholley added a commit to parisholley/mikro-orm that referenced this issue Nov 15, 2022

feat(mikro-orm#3743): replace direct require calls with util to help …

3ff552d

…with inlining

B4nan mentioned this issue Nov 18, 2022

feat(core): introduce ORM extensions #3773

Merged

B4nan added the breaking change label Nov 29, 2022

B4nan added this to the 6.0 milestone Nov 29, 2022

B4nan mentioned this issue Dec 3, 2022

feat(core): remove static require calls #3814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace direct require() calls with an indirection function #3743

Replace direct require() calls with an indirection function #3743

parisholley commented Nov 15, 2022

B4nan commented Nov 15, 2022

B4nan commented Nov 15, 2022

parisholley commented Nov 15, 2022

B4nan commented Nov 17, 2022

parisholley commented Nov 17, 2022

B4nan commented Nov 17, 2022

thekevinbrown commented Nov 18, 2022

parisholley commented Nov 18, 2022 •

edited

B4nan commented Nov 18, 2022 •

edited

B4nan commented Nov 18, 2022 •

edited

B4nan commented Nov 18, 2022 •

edited

merceyz commented Nov 18, 2022

B4nan commented Dec 3, 2022

B4nan commented Dec 3, 2022

Replace direct require() calls with an indirection function #3743

Replace direct require() calls with an indirection function #3743

Comments

parisholley commented Nov 15, 2022

B4nan commented Nov 15, 2022

B4nan commented Nov 15, 2022

parisholley commented Nov 15, 2022

B4nan commented Nov 17, 2022

parisholley commented Nov 17, 2022

B4nan commented Nov 17, 2022

thekevinbrown commented Nov 18, 2022

parisholley commented Nov 18, 2022 • edited

B4nan commented Nov 18, 2022 • edited

B4nan commented Nov 18, 2022 • edited

B4nan commented Nov 18, 2022 • edited

merceyz commented Nov 18, 2022

B4nan commented Dec 3, 2022

B4nan commented Dec 3, 2022

parisholley commented Nov 18, 2022 •

edited

B4nan commented Nov 18, 2022 •

edited

B4nan commented Nov 18, 2022 •

edited

B4nan commented Nov 18, 2022 •

edited