Fix for ES Module detection using npm@7 (#4295)

[richardsimko]

No description provided.

[kibertoad]

@richardsimko Bunch of previous tests seem to be failing, see https://github.com/knex/knex/pull/4296/checks?check_run_id=1904682577

[richardsimko]

Yeah I don't think my approach to mocking the env vars was good. I'll try another way but if that doesn't work I'm not sure it's possible to add tests for this code in a good way.

[kibertoad]

@richardsimko Unit tests are fixed, but it looks like something in migration loading logic might be broken, judging by non-mysql integration tests.

[richardsimko]

I think what's most likely happening is that I've uncovered latent issues where importFile was not used with await. import(filePath) is an async function and always was - but this function was only used in the ESM case which doesn't seem to be commonly used. Now importFile is always async and this is how that issue manifests. Fixing this would require a lot more in-depth knowledge of knex than what I have but if you have any pointers as to where this might be an issue I'm all ears.

[richardsimko]

importFile seems to be used here

knex/lib/migrations/migrate/sources/fs-migrations.js

Line 84 in 2cc3247

return importFile(_path);

Which in turn is used in many places without await:

knex/lib/migrations/migrate/Migrator.js

Line 493 in 2cc3247

const migrationContent = this.config.migrationSource.getMigration(

knex/lib/migrations/migrate/Migrator.js

Line 145 in 2cc3247

this.config.migrationSource.getMigration(migrationToRun)

knex/lib/migrations/migrate/Migrator.js

Line 84 in 2cc3247

const migrationContents = this.config.migrationSource.getMigration(

[kibertoad]

@richardsimko Yeah, noticed same thing:

knex/lib/migrations/migrate/sources/fs-migrations.js

Line 84 in b20a31f

return importFile(_path);

seems to be the only place where we importFile without awaiting, so I would review all cases where getMigration is used to ensure we await properly.

[richardsimko]

I can slap awaits on all the above mentioned locations but in my experience fixing these types of issues is never that easy 😅

[kibertoad]

@richardsimko You might need to make few more functions async (e. g. waterfallBatch), but that should do it. We have pretty good coverage for migrations at this point, if something breaks down, you'll know immediately.

[richardsimko]

It worked 🎉 I added a note in the changelog as well. Do you want me to squash the commits?

[kibertoad]

@richardsimko Thanks a lot!
Squashing is not worth the effort, I'll squash on merge anyway.

[richardsimko]

@kibertoad Is there anything I have to do in order to get this backported to 0.21.x?

[kibertoad]

@richardsimko There is a 0.21 branch, just open a PR targetting that.

[richardsimko]

Thanks, I opened #4308