feat(core): improved configuration loaders #6333
Conversation
* Add new configuration loaders utilities; * Support new transpilers: jiti and tsx; * Add in-code documentation for CLI Settings interface; * Prepend binaries in git hooks with yarn to use local versions; * Add new common utilities: tryModule and requireDefault; * Add new error type TryModuleError for internal use only; * Add tests for these two utilities; * Add NodeJS namespace to ESLint to prevent false-positives;
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6333 +/- ##
==========================================
- Coverage 99.80% 99.67% -0.13%
==========================================
Files 266 267 +1
Lines 19205 19293 +88
Branches 4829 4273 -556
==========================================
+ Hits 19167 19231 +64
- Misses 38 62 +24 ☔ View full report in Codecov by Sentry. |
I believe those were already resolved to the locally installed versions. I don't have them installed globally and it works just fine.
This feels weird to me, at least |
I got an error when I tried to commit for the first time saying it can't find
This is still supported if the For backward compatibility reasons I'll keep Also, I found that |
The new For example, when set to |
|
Ok good. So the PR as is should be good for 6.x release, no BCs, right?
It is mentioned here and here, but feel free to mention it on other places. The thing is, you are not supposed to use this outside of test, and the value is inferred automatically, but having some explicit docs around it won't hurt for sure.
You mean the settings in package.json? For that we have Also, does this work with ESM projects? Will the regular
Weird, for me it works fine (and I just verified I do not have them installed globally), but I dont mind the changes you did. |
Yes, and I will cover my code with test to make sure everything works as expected + since you have integration test for CLI, I should be able to catch potential bugs and regression in the process. But the code itself should behave the same: ts-node is the default, and old flags supported and will take precedence if Also, I'm planning to use fixtures for my tests, just like you do for tests against databases, and as I already did in my proposal, so everything will be tested against real configs.
Everything will be handled by either jiti, or tsx. They offer programmatic API that does not affect the whole runtime, unlike with ts-node, which behaves like loader or require hook. So, yes, a single entry point should be enough, and I already tested it against both CJS and ESM in my proposal. I will port these tests to Mikro ORM too. :) See: |
| */ | ||
| function loaderNameFromEnv(value?: string): Loaders { | ||
| const maybeBoolean = value?.toLowerCase(); | ||
| if (!maybeBoolean || ['false', 'f', '0', 'no', 'n', ''].includes(maybeBoolean)) { |
There was a problem hiding this comment.
we use different logic for reading bools from env vars, I would like to stay consistent with that. its used in this very file inside loadEnvironmentVars:
i don't think we need to care about y/n or yes/no (or we should respect them universally, not just here). the code should be reused ideally (we can move the current helpers out of there to the top level functions).
There was a problem hiding this comment.
we use different logic for reading bools from env vars, I would like to stay consistent with that. its used in this very file inside loadEnvironmentVars
Yes, I saw these. In this case I don't need to convert the value to a boolean. I just need to see if it can be converted. But I agree, that makes sense. So I'll just move these helpers outside of Utils class and and use in my comparison like so:
if (!value || bool(value) === false) {
// ...
}There was a problem hiding this comment.
I updated this code, but I can't just use bool function for type casting for loader option, because it can be a boolean, or a string.
| } | ||
|
|
||
| // Return `undefined` if the `value` can be converted to `true`, so behaviour will be the same as in createLoader | ||
| if (['true', 't', '1', 'yes', 'y'].includes(maybeBoolean.toLowerCase())) { |
packages/core/src/utils/Utils.ts
Outdated
| */ | ||
|
|
||
| export class TryModuleError extends Error { |
There was a problem hiding this comment.
| */ | |
| export class TryModuleError extends Error { | |
| */ | |
| export class TryModuleError extends Error { |
also maybe think of a better name? but i guess we can keep this too, it just feels weird to have the Try prefix in an error name.
and move this to the https://github.com/mikro-orm/mikro-orm/blob/678119475cee860dd4684c9f4d903a3d4028b8a3/packages/core/src/errors.ts file
btw why is it internal, do we always catch it internally or can it bubble to the user? if it can, i wouldn't mark it as internal
There was a problem hiding this comment.
btw why is it internal, do we always catch it internally or can it bubble to the user? if it can, i wouldn't mark it as internal
It is for internal use only. We will catch it and use its data (that's what is the specifier field for) to display a message asking for ts-node/jiti/tsx installation. Unlike Node.js' errors it can be tested with instanceof and type casting is not needed anymore.
I think tryModule and requireDefault should also be inaccessible from @mikro-orm/core package, since they supposed to use internally.
There was a problem hiding this comment.
I think tryModule and requireDefault should also be inaccessible from @mikro-orm/core package, since they supposed to use internally.
Makes sense, as long as it won't be part of the barrel file, it shouldn't be exported. I guess we should rename them to exports.ts or similar, I don't want to use them internally (it can cause issues with cycles), they should be there currently only for this use case.
edit: but you'll have to adjust the barrel file, I think it now reexports this whole file.
There was a problem hiding this comment.
but you'll have to adjust the barrel file, I think it now reexports this whole file.
Which one? The Utils? Yup, You can see how I access my utils in the tests.
By the way, is the errors.ts also a barrel file? Some of the errors must be part of the public API surface, so you probably re-export them too, and I want TryModuleError class (or whatever the name will be at the end) to be inaccessible outside of @mikro-orm/core, since it's for internal use only.
There was a problem hiding this comment.
Which one?
By the way, is the errors.ts also a barrel file?
Barrel files reexport things, so in that sense, nope, its not. But we reexported all its contents as part of the upper barrel file.
mikro-orm/packages/core/src/index.ts
Line 16 in 18a425c
Maybe move all of this into the loaders.ts file and don't export it at all?
There was a problem hiding this comment.
Sounds good, I'll move all these new utilities into loaders.ts, and if you'll ever need tryModule or requireDefault, you'll just change the code.
There was a problem hiding this comment.
Moved this error to loader.ts. I don't think the name matters quite a lot, but I can rename it when I figure out how to test this error's appearance (I think it can be done by mocks if I throw NodeJS.ErrnoError instead of returning the exports?), sure.
packages/core/src/utils/loader.ts
Outdated
| return loader; | ||
| } | ||
|
|
||
| const tsNode = requireDefault(await tryModule(import('ts-node'), { |
There was a problem hiding this comment.
I wonder if this won't be a problem for bundlers, ts-node is not a peer dependency and I believe bundlers will try to bundle it this way. That's why we use the tryRequire which won't suffer from it, since bundlers won't see a literal import of a package, they see just an import of some unknown dependency.
There was a problem hiding this comment.
Hm. I never though about that. I will see if that is gonna be the problem. Maybe there's a way (universal, hopefully) to ask bundlers to skip bundling some of the dependencies for package authors. Good catch though!
There was a problem hiding this comment.
AFAIK wrapping the import inside a helper function and passing the module name dynamically is enough.
There was a problem hiding this comment.
I see, but tryModule is not only catches ERR_MODULE_NOT_FOUND error, it also returns correct types, unlike Utils.tryRequire, which returns any. If there's no way to keep bundlers from bundling these calls - I will figure out another solution how to have both types and keep dependencies from bundling.
There was a problem hiding this comment.
I don't think you can have both. Either it will break bundlers (and will require config changes to mark those as external in users' projects), or it will be typed to any.
There was a problem hiding this comment.
Found something for Vite: https://vite.dev/guide/ssr.html#ssr-externals
It will not include @mikro-orm/* packages (as well as any other package from node_modules) to SSR bundle, but that's just Vite.
There was a problem hiding this comment.
I'm gonna rewrite this function as you suggested for backward compatibility, but I'll keep types for better DX. I don't think TS will keep them in .d.ts, so this should not be a problem.
But I think this is something to consider for v7:
- Move configuration loader to its own package. I can see that the current ConfigurationLoader class is used in quite a few places. We can also reuse these utilities to implement directory-based discovery and make this package official way do do it (if user's codebase is not using any bundler, see below).
- Deprecate and remove
MikroORM.init()without arguments - this way the new package will be part of CLI, or user's code if they want to use directory-based discovery. - Write official recipe for directory-based discovery when using MikroORM with bundlers, at least Vite (https://vite.dev/guide/features.html#glob-import) and Webpack (https://webpack.js.org/api/module-methods/#dynamic-expressions-in-import) have their own way to bundle code with dynamic imports. Afaik, it's not even possible to do it with Mikro ORM's own APIs, because bundlers will just ignore utilities, like
Utils.tryRequireandUtils.dynamicImport. If you want to support bundlers, then let's make it official :)
There was a problem hiding this comment.
Updated tryModule implementation so it will accept specifier as the first argument, instead of promise returned by import(), now it shouldn't be a problem. Feel free to close this conversation if you don't have any thoughts on my previous comment.
There was a problem hiding this comment.
Deprecate and remove MikroORM.init() without arguments - this way the new package will be part of CLI, or user's code if they want to use directory-based discovery.
There is one use case for the method even without the dynamic config loading - using only env vars. It's not a universal solution, but most things can be configured that way. But it requires folder-based discovery to work in the first place. But I guess people could just do init({}) if they want that behaviour.
Otherwise, I would be fine with telling people to just import the config file and pass it explicitly, it's one additional line, otherwise it only has benefits.
Note that folder-based discovery is one thing, but we still do similar stuff for seeders and migrations. I don't think we can just drop that part, so we won't get rid of those issues completely just by moving the discovery part.
it's not even possible to do it with Mikro ORM's own APIs
You can use bundlers just fine, but you need to configure them properly. I am not sure if that will change with what you are proposing. But I was never a huge fan of bundling on backend, don't have much experience with that.
There was a problem hiding this comment.
Note that folder-based discovery is one thing, but we still do similar stuff for seeders and migrations. I don't think we can just drop that part, so we won't get rid of those issues completely just by moving the discovery part.
Yes, but aren't they separated packages? Do they responsible for loading migrations and seeders?
But I was never a huge fan of bundling on backend
I couldn't agree more, but I usually use Mikro ORM in full-stack projects, where both backend and frontend are the same app and it's built with one of the so called "meta frameworks", and all of them use one of the bundlers. Like, Qwik, SvelteKit and React Router v7 (ex Remix) use Vite.
I forgot to mention one important thing: As long as we have to support ts-node - this might not work in a single CLI entry point, so in v6 we sure have to keep |
Co-authored-by: Martin Adámek <banan23@gmail.com>
Removing it as part of v6 would be a BC, so that's out of question anyway. |
…n auto for truthy value
|
Now the tests are failing because Jest cannot import Edit: Wait, no, the error occurs when tsc validates tests against Edit 2: I think we can use dynamic import to get the types (as tsc suggests), but your ESLint config prohibits it :) Can I disable or change the rule for this file? |
Which one? We use dynamic imports in the codebase, e.g. here. |
This one: https://typescript-eslint.io/rules/consistent-type-imports/ The error says following: So, from what I understand, I need to replace import type * tsxApi from "tsx/esm/api"with Type TsxApi = typeof import("tsx/esm/api") // Except, I will use it right in `tryModule` type parameter |
Nope, that didn't help :) |
|
|
Because I want to see if we do something wrong when integrating 3rd party libraries. AFAIK you can end up with issues without this, that you won't see but people can see if they don't use I am fine with having that disabled for tests. |
I mean, it's not like you can do about 3rd parties.
The documentation says this: https://www.typescriptlang.org/tsconfig/#skipLibCheck I'm not sure what they mean by
Will it verify public-facing types from 3rd party libraries, or just the source code? Anyway. Turns out tsc will also fail when I try and build the packages, with the same error. But at the same time the |
|
Also, I think tsc will apply same tsconfig.json for 3rd parties as in mikro orm which might cause the problem in the first place - 3rd parties may have different configs, but their code is will valid. tsc looks at their .d.cts file, which imports Edit: Found this discussed on stack overflow: https://stackoverflow.com/questions/52311779/usage-of-the-typescript-compiler-argument-skiplibcheck
|
|
I have this error when I'm using imports in my tests (I'm trying to import .mts file): So, Jest does not support esm by default? Also, this error with mts files: tsc can't pick up mikro orm typings in esm for some reason (even if I add package.json with {type: "module"} and configure tsc, just for fixtures). I don't see such problem when I install it from npm and use in my esm projects (for example - I don't have this problem in my proposal's repo). Weird. I'll probably skip tests for esm for now, since Mikro ORM seem to be configured as CJS package, but they should work. |
|
I believe this is also Jest-related issue. I get it only with tsx, though. This probably related: jestjs/jest#9580 I will skip tsx tests for now, and see if I can do anything about it. The loader should still work though. I am not much familiar with Jest, so any help from you will be appreciated. |
…ynamicImport in loaders
| @@ -39,6 +67,9 @@ export class ConfigurationLoader { | |||
| */ | |||
| static async getConfiguration<D extends IDatabaseDriver = IDatabaseDriver, EM extends D[typeof EntityManagerType] & EntityManager = EntityManager>(validate: boolean, options?: Partial<Options>): Promise<Configuration<D, EM>>; | |||
| static async getConfiguration<D extends IDatabaseDriver = IDatabaseDriver, EM extends D[typeof EntityManagerType] & EntityManager = EntityManager>(contextName: boolean | string = 'default', paths: string[] | Partial<Options> = ConfigurationLoader.getConfigPaths(), options: Partial<Options> = {}): Promise<Configuration<D, EM>> { | |||
| const settings = ConfigurationLoader.getSettings(); | |||
| const basePath = options.baseDir ?? process.cwd(); | |||
There was a problem hiding this comment.
I'm not sure what is the baseDir option for, but the projectRoot argument is meant to be configurable (for tests and for use), so I had to get it from somewhere and not just re-use process.cwd().
|
@B4nan I might need your help with Also, I've tried to pass base path by overriding I know you test against mocks, but I want to make sure my loaders integrated properly so I won't break anything in v6, so I run my tests against real config files. |
|
I wonder if it would be better to target v7 here, I've already set up the branch and I am almost there with ESM rewrite using vitest instead of jest. |
|
Maybe, I don't mind. But for v7 this PR will probably be useless, except for the |
|
Why useless? It adds support for other TS support providers, or did I miss something?
I am not completely opposed to doing that but so far I haven't seen a good enough reason. |
I meant, it would probably need to be rewritten, because it might not fit in its current state in a future, and as I said - it would depend on whether or not it will be put into a separate package. The loaders implementation is completed thought, no matter how it will be integrated - it tested and it should work.
Well, as I already said, I think it might help to improve support for bundlers, if we clear the core package from dynamic import and things like |
|
Btw, do we still need ts-node support if we move this to v7 instead? |
|
I just found out that jiti having problems when it encounters class properties without initializer: unjs/jiti#57 So, I guess this pr is stuck anyway until and if they fix the problem. tsx does not have this problem, but then again - they don't support decorators metadata. The issue is quite old, so I don't expect maintainers will respond, and if they don't - I will open an issue there. I'll wait until the fix arrives before moving forward with the pr. |
|
I started testing out the ESM rewrite (#6403) and the ts-node solution we have now is not working at all, so there is your answer - ts-node is going out from v7 right now :] I just swapped it with |
|
Glad to hear that. I'll remove ts-node support then, when we will move this PR forward. Hope jiti lans PR with fix for the issue I was talking about in my previous comment, otherwise the only two loaders left will be tsx and native. Or maybe I can replace ts-node loader with swc (as I initially proposed, in addition to other options), as far as I can tell, the implementation will be the same mikro-orm/packages/core/src/utils/ConfigurationLoader.ts Lines 236 to 239 in a146959 |
Details
This PR implements improves configuration loading with support for new transpilers, like jiti and tsx. It also includes documentation updates, reflecting these changes. This PR also deprecates some of the old CLI settings:
useTsNode,preferTs, andalwaysAllowTsin favor of a single new option calledloader, and this new option allows to opt-in to a specific transpiler, or use auto-detection (the default), or opt-out to runtime's nativeimport()(when set to either false ornative).Notable changes
ts-node,jiti,tsx, andnative;tryModule- it resolves a promise, returned byimport()and catches the errors with theERR_MODULE_NOT_FOUNDcode, then wraps it into aTryModuleError, so it can be handled later. Unlike existent utilities (Utils.dynamicImport and Utils.tryRequire), the resulting value has correct types (though it only supports theimport()- it can be easily modified to supportrequire()function as well);requireDefault- resolves thedefaultmember from a module's exports while also providing correct types;useTsNode,preferTs, andalwaysAllowTsin favor ofloaders;ConfigurationLoaderclass;ConfigurationLoaderintegration:ConfigurationLoader.getConfigurationmethod;I also updated git hooks to use local binaries of
lint-stagedandcommitlint, because there's no point to have those installed globally (yarn can pick up both local and global installations), nor to assume everyone has these by default.