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

chore(deps): Upgrade umzug to v3 #742

Closed
wants to merge 5 commits into from

Conversation

thomaschaaf
Copy link
Contributor

No description provided.

},
"devDependencies": {
"@mikro-orm/core": "^4.0.0-rc.1"
"@mikro-orm/core": "^4.0.0-rc.1",
"@types/validator": "^13.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

else tests failed 🤷

"@mikro-orm/core": "^4.0.0-rc.1"
"@mikro-orm/core": "^4.0.0-rc.1",
"@types/validator": "^13.1.0",
"sequelize": "^6.3.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

else tests failed 🤷

Copy link
Member

Choose a reason for hiding this comment

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

oh jeez, and i though they will make it right when migrating to TS 🤷. nope, i dont want sequalize as dependency, not even dev one.

what were the failures? this sounds like something to report to their project

will try the upgrade myself too to see it in action, if its just about those tiny changes

@thomaschaaf thomaschaaf changed the title Upgrade umzug to v3 chore(deps): Upgrade umzug to v3 Aug 13, 2020
@thomaschaaf
Copy link
Contributor Author

@B4nan Okay I am sorry I will stop with the upgrade then. There are some unit tests failing and I'd have to do some deeper digging.

@B4nan
Copy link
Member

B4nan commented Aug 13, 2020

no need to be sorry, its not even your project :] i will take a look, maybe it could work just by having @types/sequalize? I see them included too when looking at current node_modules.

@B4nan
Copy link
Member

B4nan commented Aug 13, 2020

What were the issues without having sequalize and @types/validator included? I did the changes as you did, without including them, and 2 tests are failing, but nothing about those dependencies, the error i see is:

TypeError: The "path" argument must be of type string. Received an instance of Array
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received an instance of Array
    at validateString (internal/validators.js:120:11)
    at Object.basename (path.js:1156:5)
    at Object.nameFormatter (/usr/local/var/www/b4nan/mikro-orm/node_modules/umzug/src/migration.ts:21:44)
    at Migration.testFileName (/usr/local/var/www/b4nan/mikro-orm/node_modules/umzug/src/migration.ts:75:55)
    at /usr/local/var/www/b4nan/mikro-orm/node_modules/umzug/src/umzug.ts:352:40
    at Array.find (<anonymous>)
    at Umzug._findMigration (/usr/local/var/www/b4nan/mikro-orm/node_modules/umzug/src/umzug.ts:352:28)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

And that is caused by this line in the tests:

await migrator.up({ migrations: [migration.fileName] });

Which was working fine with umzug 2.

@thomaschaaf thomaschaaf reopened this Aug 13, 2020
@thomaschaaf
Copy link
Contributor Author

I tried only importing the typing but that doesn't work either because it requires sequelize ^5 for the storage adapter.

@merceyz
Copy link
Contributor

merceyz commented Aug 13, 2020

They have two undeclared dependencies right here https://github.com/sequelize/umzug/blob/493e4e8d803b327655ec4b0322bf47cd248b747a/src/storage/sequelize.ts#L2-L3 granted they're only used for types so runtime should work

@B4nan
Copy link
Member

B4nan commented Aug 13, 2020

Let's close this now, as #741 is working without the need to ugprade, so better to wait for stable release (that will hopefully have that hidden dependency fixed).

@B4nan B4nan closed this Aug 13, 2020
@thomaschaaf thomaschaaf deleted the upgrade-umzug-to-v3 branch August 13, 2020 14:18
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.

None yet

3 participants