fix: ensure implicit config options load automatically

[lsh4711]

Issue

Since version 6.3.14-dev.57 of @mikro-orm/core, modifications in the ConfigurationLoader related code have led to the paths is not iterable error if a config is not explicitly provided to MikroOrmModule.forRoot().

Changes

• Adjusted to align with the new getConfiguration() signature.
• Prefer configPathsFromArg() when auto-loading options(but this has become deprecated).
• Added test code to ensure options auto-loading.

[B4nan]

cc @boenrobot maybe we should make the signature backwards compatible so we don't break this integration

it's internal method, so I don't mind it being a bit ugly, keeping the first argument as a boolean that doesn't do anything in the newer versions

[boenrobot]

Probably better to make one release without this that locks the version to 6.3, and then a new one with the changes, requiring 6.4+...

This would also enable the lack of duplicating the deprecation notice... it could be a new major of this package that simply removes support for it.

[B4nan]

I would rather go with what I said above, we don't need to change the order of params in that method (only add the new params to the end), that's a small price to pay for proper back compatibility on internal API. Or even detect old vs new signature. You never know if some other integration modules that we don't maintain use things like this, better to stay safe. We can clean things up with v7.

This would also enable the lack of duplicating the deprecation notice... it could be a new major of this package that simply removes support for it.

What I want would mean no changes in this package. I definitely don't want to do a major bump to misalign the versions between it and the ORM itself.

[boenrobot]

I think perhaps this BC can be achieved by making ConfigLoader.getConfiguration() be overloaded, and based on whether the first arg is boolean, trigger the old behavior. The boolean signature can be marked deprecated in js docs to give 3rd party integrations a notice.

I can make a PR for this first thing in the morning (around 12-13 hours from now...).

[B4nan]

I think perhaps this BC can be achieved by making ConfigLoader.getConfiguration() be overloaded, and based on whether the first arg is boolean, trigger the old behavior. The boolean signature can be marked deprecated in js docs to give 3rd party integrations a notice.

Yep, that's what I meant when I said we could detect the old vs new signatures.

I can make a PR for this first thing in the morning (around 12-13 hours from now...).

Sounds good, no rush, we can leave the release for tomorrow evening or later, not a huge deal. Thanks!

[B4nan]

@lsh4711 please test it with latest dev version (6.3.14-dev.70), should be now working thanks to mikro-orm/mikro-orm#6213

[lsh4711]

In 6.3.14-dev.70, the path issue was resolved, but it still didn't work, so I've been investigating the cause.
Here's the reproduction.

I belatedly found out that the issue does not reproduce in the test when using mocked dynamic import.

[boenrobot]

Hm... I guess I was wrong about this integration not using getConfiguration(false)... It uses it here:

nestjs/src/mikro-orm-core.module.ts

Line 159 in dd3f9e3

config = await ConfigurationLoader.getConfiguration(false);

But this can be easily fixed (without a version bump in the deps) by changing that line to
config = new Configuration({}, false);

And that line is in turn triggered by calling forRoot() without any options.

[B4nan]

But this can be easily fixed (without a version bump in the deps) by changing that line to

As I said, I don't want to fix it here, as that means people can end up with typeerrors because of updating the ORM and not this adapter (or worse, with malfunctioning). This needs to be fixed in the ORM itself before we release v6.4

[boenrobot]

Fair enough... Fixed in mikro-orm/mikro-orm#6217

[B4nan]

@lsh4711 I guess you closing this means its working correctly now? :]

[lsh4711]

Oh.. I thought the issue was resolved by now and closed this PR without checking, which was a mistake and the problem still hasn't been fixed.
As mentioned above, the same issue is still occurring both in the reproduction and on the actual server. (in 6.3.14-dev.73)

Based on my investigation, the solution I found is to use the call to ConfigurationLoader.commonJSCompat() to maintain compatibility with the behavior of the previous version.
In other words, since this code is missing, the config object that has been wrapped once more (like { default: config, ..other configs in same config file }) is being dynamically imported, so the config isn't being interpreted correctly, which seems to be why the issue persists.

[B4nan]

Ok, lets reopen till its actually fixed.

[boenrobot]

Ugh... ok... This time, before un-drafting the PR ( mikro-orm/mikro-orm#6218 ), I'm going to test it with the repro...

And on that note, I should say that the repro seems a bit incomplete... I had to add extra nestjs deps to even reach the point where the config was the problem.

@lsh4711 feel free to try yourself with that branch too.

[lsh4711]

Great, it works perfectly now. :)
Thanks to both B4nan and boenrobot!