feat(cli): allow the use of TS path mapping #554

AlexAegis · 2020-05-07T18:11:05Z

This enables the use of path aliases in the config file.

Example tsconfig.json

{
	"compilerOptions": {
		"paths": {
			"@foo": ["src/foo/index.ts"]
		}
	},
}

Example mikro-orm.config.ts

import * as foo from '@foo';

const config = foo.myFancyFunctionThatReturnsAFancyConfig();

export default config;

B4nan

also please adjust tests so we stay on 100% branch coverage (should be quite easy via unit test as you are modifying public method).

B4nan · 2020-05-07T19:27:03Z

packages/core/src/utils/ConfigurationLoader.ts

@@ -32,6 +32,14 @@ export class ConfigurationLoader {
    return {};
  }

+  static async getTsConfig(): Promise<Dictionary> {
+    if (await pathExists(process.cwd() + '/tsconfig.json')) {


we should use the path from the package.json settings (the one from getSettings method, tsConfigPath)

I see.. Then I think I've found a bug (Assuming the default settings should be applied everywhere). In CLIHelper.ts the configure function directly calls the ConfigurationLoader::getSettings method. But the defaults are set in the constructor of Configuration.

CLIHelper::configure is also used by the cli.ts itself. Shouldn't the defaults supposed to be applied there too?

If not then then it's fine. But then I need to apply the default myself.

Another thing is, that if I want to only require the Settings once then I'd have to require the tsconfig there. But it would look nicer in its own method, and anyway, eslint doesn't allow me to require into variables. But I've seen that await ConfigurationLoader.getSettings() is used a few times anyway. I suggest memoizing this function, then it would be 100% correct.

Not really a bug, you are mixing two things, package.json settings (CLIHelper.getSettings()) and ORM configuration (Configuration class). They are for different things. I don't want to adjust the ORM config anyhow - overriding stuff based on something from package.json seems just wrong. The whole point of this package.json settings is to allow to configure CLI correctly - so to let user say where the right config is and whether we want to run via ts-node or not. And to do so, one can specify where the tsconfig.json file is.

All I was asking for is to use the settings.getTsConfig as a path for the require, so as a parameter to this function. Usign the default from Configuration seems wrong, although thee default is same, the class has nothing to do with CLI itself.

package.json

B4nan · 2020-05-07T20:50:41Z

packages/core/src/utils/ConfigurationLoader.ts

@@ -32,6 +32,16 @@ export class ConfigurationLoader {
    return {};
  }

+  static async getTsConfig(): Promise<Dictionary> {
+    const settings = await ConfigurationLoader.getSettings();


as said, this should not be here and it should not depend on Configuration

what you had there before was ok, i just wanted you to prefer the path from settings.tsConfigPath if provided

Whats the problem with using the default value defined there as a fallback? The same one was there, I just didn't want to duplicate it after I saw it defined there already. Or this whole function shouldn't be there?

This whole function shouldn't be here, if you need it, move it back to CLIHelper, but without that extra call to getSettings. I said you should pass that value via parameter, not call it again.

Again, what you had there in the first commit was ok, I just wanted to have 1 line change (using the path from already loaded settings object), instead you rewrote it and made it much more complex :]

I do noticed that it doesn't really make sense to have a method like that at all. And it would make much more sense to just have this included at the config, and I did just that. But I need help because I can't make the test work. tsconfig.json should be mocked in there, but it only works if it's required two directory levels more inside, in ConfigurationLoader.ts, but if I put it in the configure method it does not call the mock.

Also, the require is in an IIFE because eslint doesn't let me require into var. And I don't want to ignore or turn off rules.

Just add eslint ignore comment there, the rule is irrelevant here, by using IIFE (its IIFE, not IFEE :]) you are just getting around it, without fixing the root cause (you are still using require()).

Oh yeah, that was a typo. Before top level async was introduced I've used it a lot.

That's why I asked you what would you prefer, ignore it explicitly or placing it in a function.

package.json

B4nan · 2020-05-07T23:34:41Z

packages/core/src/utils/ConfigurationLoader.ts

@@ -32,6 +32,16 @@ export class ConfigurationLoader {
    return {};
  }

+  static async getTsConfig(): Promise<Dictionary> {
+    const settings = await ConfigurationLoader.getSettings();


This whole function shouldn't be here, if you need it, move it back to CLIHelper, but without that extra call to getSettings. I said you should pass that value via parameter, not call it again.

Again, what you had there in the first commit was ok, I just wanted to have 1 line change (using the path from already loaded settings object), instead you rewrote it and made it much more complex :]

B4nan · 2020-05-08T09:08:47Z

I am sorry but I already spend more time giving you reviews than I would spend with implementing this myself. If you are stuck, I can finish this myself, no probs.

Generally thanks for this, I was planning to do it myself for few months already :]

AlexAegis · 2020-05-08T09:31:04Z

I don't have enough experience with Jest to fix the test, so yes it would be nice to have some help.

I already spend more time giving you reviews than I would spend with implementing this myself.

This is a little discouraging. I'd like to use this library but there is a lot to implement and I don't want to demand every little change like this, but to contribute myself.

B4nan · 2020-05-08T09:51:24Z

This is a little discouraging. I'd like to use this library but there is a lot to implement and I don't want to demand every little change like this, but to contribute myself.

Please understand that I am spending my own free time with this project. And I already spend 2+ hours with this small change, that I would have done in less then hour. I am spending dozens of hours every week like this.

So I simply said that I will rather finish this myself than spend another 2+ hours on reviews, because my time is very limited and I have many things to work on.

I am more than happy to see more contributions, but:

we should first agree on what and how (and why) should be changed. Here you send the PR right ahead, and when I asked for one simple change, you started elaborating on the why. I simply can't explain to you every decision I made.
you should be able to test things yourself and provide tests for your changes. testing things against the CI is not very welcome - every push you do is sending me email notification.
this project has high standards in terms of code style and test coverage, if you want to contribute, you must adhere to that. and again, I did offer to you I will handle the testing issue myself.

So I am sorry if you feel discouraged, but please try to understand my point of view as well.

AlexAegis · 2020-05-08T10:03:06Z

This last push is not "testing against the CI" I can run the tests myself. I just incorporated the last requested change you mentioned what I can solve. If you have the time to look at the mock issue I had please take a look at it.

B4nan · 2020-05-09T13:59:59Z

Ok so this was quite tricky, the problem was that package.json was required from ConfigurationLoader and tsconfig.json was required in CLIHelper, but both were using the same relative (mocked) process.cwd(). So the solution was to actually move the code to ConfigurationLoader.

Once again sorry if I offended or discourage you, and thanks for your contribution.

@foo