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

feat(migrations): allow specifying list of migrations #741

Merged
merged 7 commits into from
Aug 13, 2020

Conversation

thomaschaaf
Copy link
Contributor

No description provided.

packages/core/src/utils/Configuration.ts Outdated Show resolved Hide resolved
packages/migrations/src/Migrator.ts Show resolved Hide resolved
packages/migrations/src/Migrator.ts Show resolved Hide resolved
packages/core/src/typings.ts Outdated Show resolved Hide resolved
docs/docs/migrations.md Outdated Show resolved Hide resolved
docs/docs/migrations.md Show resolved Hide resolved
docs/docs/migrations.md Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
import umzug, { Umzug } from 'umzug';
import { Utils, Constructor } from '@mikro-orm/core';
import umzug, { Umzug, migrationsList } from 'umzug';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fails in the CI as well as on my end, probably bad/outdated typings? the migrationsList is in the sources, but not in the d.ts

maybe we could access it from the umzug default export, or we would need to augment the typings (via declaration merging)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried everything I could but ultimately this seems to be the only way I am able to fix this until the typings are merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could use your fork as a dependency?

or as i mentioned before, we could use declaration merging

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know how to do this. Would you mind showing how to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgtm-com
Copy link

lgtm-com bot commented Aug 13, 2020

This pull request introduces 1 alert when merging b23ef06 into 0474787 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Aug 13, 2020

This pull request introduces 1 alert when merging 16669d9 into 0474787 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@B4nan
Copy link
Member

B4nan commented Aug 13, 2020

Ok, tests are green, let's merge this, will take a look at the typings later

@B4nan B4nan changed the title Add ability to add migrations as imports. feat(migrations): allow specifying list of migrations Aug 13, 2020
@B4nan B4nan merged commit 5a0f2a6 into mikro-orm:master Aug 13, 2020
@thomaschaaf
Copy link
Contributor Author

I fixed it here: c380d99

@B4nan
Copy link
Member

B4nan commented Aug 19, 2020

Hmm damn umzug and their mediocre typings...

node_modules/@mikro-orm/migrations/Migrator.d.ts:4:16 - error TS2665: Invalid module name in augmentation. Module 'umzug' resolves to an untyped module at '/usr/local/var/www/vision.gl/vision-api/node_modules/umzug/lib/index.js', which cannot be augmented.

4 declare module 'umzug' {
                 ~~~~~~~


Found 1 error.

What about that PR you made against definitely typed, is that merged already?

@merceyz
Copy link
Contributor

merceyz commented Aug 19, 2020

If you move that declare module 'umzug' to it's own file it should work
The fix is to make @types/umzug a dependency of @mikro-orm/migrations

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 this pull request may close these issues.

3 participants