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

Running Migrations with es modules #2631

Closed
PodaruDragos opened this issue Jan 12, 2022 · 17 comments · Fixed by #2641
Closed

Running Migrations with es modules #2631

PodaruDragos opened this issue Jan 12, 2022 · 17 comments · Fixed by #2641

Comments

@PodaruDragos
Copy link
Contributor

Describe the bug
There is no way of running migration using the CLI with es modules.

Stack trace

mikro-orm migration:create --initial

Create new migration with current schema diff

Options:
  -b, --blank    Create blank migration                                [boolean]
  -i, --initial  Create initial migration                              [boolean]
  -d, --dump     Dumps all queries to console                          [boolean]
  -p, --path     Sets path to directory where to save entities          [string]
  -v, --version  Show version number                                   [boolean]
  -h, --help     Show help                                             [boolean]

Error: No entities found, please use `entities` option
    at Configuration.validateOptions (/..../mikro-es-modules/node_modules/@mikro-orm/core/dist/utils/Configuration.js:169:19)
    at new Configuration (/...l/mikro-es-modules/node_modules/@mikro-orm/core/dist/utils/Configuration.js:24:18)
    at Function.getConfiguration (/.../mikro-es-modules/node_modules/@mikro-orm/core/dist/utils/ConfigurationLoader.js:55:20)
    at async Function.getORM (/.../mikro-es-modules/node_modules/@mikro-orm/cli/dist/CLIHelper.js:19:25)
    at async Function.handleMigrationCommand (/.../personal/mikro-es-modules/node_modules/@mikro-orm/cli/dist/commands/MigrationCommandFactory.js:88:21)

To Reproduce
Steps to reproduce the behavior:

  1. Clone this repo repo

Expected behavior
I would like to be able to run migration-create --initial.

Versions

Dependency Version
node 17.3.0
typescript 4.5.4
mikro-orm 5.0.0-dev.604
your-driver postgres
@B4nan
Copy link
Member

B4nan commented Jan 12, 2022

I believe you can't use folder based discovery if you want native ESM. In other words, you can't use env vars for the CLI as that implies folder based discovery - you will need a CLI config file where you use entities: [A, B] just like in your app.

tbh I don't recall why we had this limitation in the first place, I believe all dynamic requires are now gone from the discovery, so will definitely check that

right, i remember now, we compile the code to CJS, so effectively we still have require calls there even tho not in the TS source code

edit: obviously the validation itself is correct, you are trying to run the CLI without configuring where it should look for entities

also note that MIKRO_ORM_MIGRATIONS_PATTERN is no longer valid (not 100% sure what version tho, but latest dev is using glob instead of pattern

https://mikro-orm.io/docs/next/upgrading-v4-to-v5/#migrationspattern-is-removed-in-favour-of-migrationsglob

@PodaruDragos
Copy link
Contributor Author

PodaruDragos commented Jan 12, 2022

I believe you can't use folder based discovery if you want native ESM. In other words, you can't use env vars for the CLI as that implies folder based discovery - you will need a CLI config file where you use entities: [A, B] just like in your app.

Will try it out. Thanks.
Didn't work tho, but I see i can do await connection.getMigrator().createMigration('./src/migrations/', false, true); and that works.

I think i'll just give up on this es-modules thing ( and just downgrade some of the projects that have moved to esm ) since that is the sole reason i am even trying this out.

right, i remember now, we compile the code to CJS, so effectively we still have require calls there even tho not in the TS source code

Yeah that's that. I remeber reading about something that you added via gen-esm-wrapper. Can that be used in any way ?

@B4nan
Copy link
Member

B4nan commented Jan 12, 2022

Yeah that's that. I remeber reading about something that you added via gen-esm-wrapper. Can that be used in any way ?

That has a different purpose, to allow using named imports. To support dynamic imports we will need to wait for TS 4.6+ most probably, as they removed the node12 thingy from TS 4.5

Maybe I could had this as part of some postprocessing - after tsc does the job, I could dynamically patch the compiled files to add the dynamic imports back to where they belong. It's ugly, but it works (used this technique on another project). Another ugly workaround is via eval.

edit: maybe the eval trick would be actually worth it, we could move it to a helper in utils class, will give that a try as I would very much like to support ESM as much as possible in v5

@PodaruDragos
Copy link
Contributor Author

PodaruDragos commented Jan 12, 2022

edit: maybe the eval trick would be actually worth it, we could move it to a helper in utils class, will give that a try as I would very much like to support ESM as much as possible in v5

How would that work tho ? using a normal .env file of by using a config.file ?

Edit: never mind, i realized it has nothing to do with that. Thanks for talking the time to look into this.

@B4nan
Copy link
Member

B4nan commented Jan 12, 2022

We would have dynamic imports in the compiled ORM code, so folder based discovery would work out of box probably. So you could have just the .env file.

@B4nan
Copy link
Member

B4nan commented Jan 12, 2022

Just tried and it works fine for the compiled files, in CJS projects in behaves a bit weirdly but that would be workaroundable. But looks like it breaks with ts-node so it breaks tests that uses folder based discovery. I can offer doing this conditionally, based on an env var, e.g. MIKRO_ORM_DYNAMIC_IMPORTS=1. (edit: added MIKRO_ORM_DYNAMIC_IMPORTS to master)

But there is another gotcha, which is running migrations. They also work with file system, and use require instead of dynamic import. But we use umzug for this, and it does not allow async callback in that place, so we can't just swap this with the same dynamic import helper. We could probably replace umzug with custom implementation, as it does not bring that much added value. But it is a chore I don't want to spend my time on right now, at least not for free.

In general seems like the ecosystem is not ready yet, too many obstacles for no actual benefit (apart from being stuck on some older versions of things that moved forward of course, which sucks).

@B4nan
Copy link
Member

B4nan commented Jan 12, 2022

B4nan added a commit that referenced this issue Jan 12, 2022
By setting `MIKRO_ORM_DYNAMIC_IMPORTS` we now get a dynamic import even
tho we still compile to CommonJS. This enabled support for folder based
discovery when using ES modules.

Related: #2631
@PodaruDragos
Copy link
Contributor Author

In general seems like the ecosystem is not ready yet, too many obstacles for no actual benefit (apart from being stuck on some older versions of things that moved forward of course, which sucks).

yeah, maybe typescript 4.6 will have what we need. Anyways thanks for looking into this. Maybe umzug will add the async callbacks needed and maybe then it won't be a such a big change. ( providing a custom implementation is really not needed since we can just wait till we can use proper CJS/ESM imports ).

If you want feel free to close this and open/reopen if ever needed.

@B4nan
Copy link
Member

B4nan commented Jan 12, 2022

actually umzug does support this, although a bit more verbose than just swapping require/import

https://github.com/sequelize/umzug/blob/ad50a474bf872480786d48e60e08f03427a9386f/examples/2.es-modules/umzug.mjs#L17-L33

@B4nan
Copy link
Member

B4nan commented Jan 12, 2022

Looking good!

MIKRO_ORM_DYNAMIC_IMPORTS=1
MIKRO_ORM_ENTITIES=dist/entities/*.js
MIKRO_ORM_ENTITIES_TS=src/entities/*.ts
MIKRO_ORM_MIGRATIONS_PATH=dist/migrations
MIKRO_ORM_TYPE=postgresql

image

I guess we also need to introduce MIKRO_ORM_MIGRATIONS_PATH_TS so it can work for ts-node too out of box with pure env CLI config too.

@PodaruDragos
Copy link
Contributor Author

Looking good!

that's great !

@B4nan B4nan closed this as completed in 072f23f Jan 12, 2022
@PodaruDragos
Copy link
Contributor Author

PodaruDragos commented Jan 13, 2022

hey Martin, i've pushed to the same repo

1st problem, if you have any kind of relation when running migrations it won't work if the extension is not specified (ex: B.js). This happens only for entities while running migrations, no other node files. If you put the extension to .js it work tho.

2nd problem, migrations are always created in the dist( with ts extension ) folder and not in src, maybe i missed something from the commit. if you have the time, any type of help is much appreciated.

EDIT: tbh 1st problem is really not a problem since we can always just add the .js extension and the second one i think i just probably didn't do the config correctly.

@B4nan
Copy link
Member

B4nan commented Jan 13, 2022

Well, we can emit the migrations to "correct place" based on the emit option, that would be minor thing. But then it will emit the TS migration to src (if you keep emit: 'ts' which is the default), while running it won't work unless you first compile it yourself - at least if you want the CLI to run compiled migrations which I believe you want, right?

@PodaruDragos
Copy link
Contributor Author

I did built the code and the tried to run migrations.

at least if you want the CLI to run compiled migrations which I believe you want, right?

that's ideal yes, but i am using ts-node so i can also run the ts migrations aswell

@B4nan
Copy link
Member

B4nan commented Jan 13, 2022

My point is, that if you set up the CLI to run in JS context, you will need to manually compile things first. The CLI can't do this dynamically, you need to provide the configuration so it knows what it should do. To run CLI in ts-node context, you would have to adjust your package.json to enable this behaviour (that's the useTsNode thing). If I do this, I start to get things like TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/adamek/htdocs/mikro-orm/mikro-es-modules/src/entities/A.ts, which I believe is the broken ts-node support I was referring to above.

Note that I am able to get around that by using this:

node  --experimental-specifier-resolution=node --loader ts-node/esm ./node_modules/.bin/mikro-orm migration:create --initial

So again, I can offer the fix of where TS migration gets generated, with that you can generate TS, compile to JS and use the CLI to run the JS migrations. Will ship that in a minute.

const storage = new AsyncLocalStorage<EntityManager>();

Btw there is also no point of handling the ALS context explicitly, as ALS is used by default in v5.

@PodaruDragos
Copy link
Contributor Author

This works really good, thanks !

@PodaruDragos
Copy link
Contributor Author

Hey Martin,
I've run into problems again 😢. Running the same repo on windows will give this error.

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs.

After some debugging it seems it fails here

async getEntityClassOrSchema(path, name) {
        const exports = await Utils_1.Utils.dynamicImport(path);
        

And the error most likely comes from return Function(`return import('${id}')`)();.

This esm modules.. 😵‍💫

I would assume it needs to be somehow relative when using windows and es modules since absolute paths are not supported on windows. issue

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 a pull request may close this issue.

2 participants