Added support for native Node ES6 modules #211
Conversation
|
You can pass in the |
|
Coverage restored 🎉 |
| @@ -564,6 +566,10 @@ export function createContainer<T extends object = any, U extends object = any>( | |||
| return resolver.resolve(container) | |||
| } | |||
|
|
|||
| function loadModules<ESM extends boolean = false>( | |||
There was a problem hiding this comment.
Are these 2 signatures not the same? You should be able to get away with just using this one.
There was a problem hiding this comment.
Sadly not, I'm not quite sure why but if I try to use only the definition with the optional return it doesn't work claiming it doesn't match the interface:
Type 'Promise<{ options: ContainerOptions; cradle: T; inspect: (depth: number, opts: any) => string; cache: Map<string | symbol, CacheEntry<any>>; loadModules: <ESM extends boolean = false>(globPatterns: (string | ... 1 more ... | [...])[], opts: LoadModulesOptions<...>) => ESM extends false ? AwilixContainer<...> : Promi...' is not assignable to type 'ESM extends false ? AwilixContainer<any> : Promise<AwilixContainer<any>>'.ts(2322)
And of course using only the type union doesn't work since it doesn't match the condition. This suggests the same issue: https://stackoverflow.com/a/55059896/1091402
load-modules.ts has the same issue.
|
I’ll take a look when I’m back Monday, thanks! |
|
How come Have you by any chance tested that the Browser builds work after this change? |
|
It's external because it's just copied as-is, it should not be touched by the TS compiler since it will convert The browser build works to the extend that there is a file generated, but I haven't used it for anything. That said since |
|
Merged and released as v4.3! 🥳 Thank you! |
| ): Promise<LoadModulesResult> { | ||
| const importPromises = [] | ||
| for (const m of modules) { | ||
| importPromises.push(dependencies.require(m.path)) |
There was a problem hiding this comment.
I got a bug in this part when using in windows
Uncaught Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol
While debuging i could fix by adding these lines
const isWindows = process.platform === 'win32';
const _path = isWindows? `file://${m.path}`: m.path;
importPromises.push(dependencies.require(_path))
There was a problem hiding this comment.
Hi @jeffijoe and @richardsimko
Should i open this comment as a new issue, did you get what i mean?
I used the following glob that throws that error in windows
There was a problem hiding this comment.
Hi! I thought node would handle that automatically! It does on macOS at least, since the path that import() gets called with (Which is at the top of the stack from this call) is a file:// URL. I sadly don't have access to a Windows computer to test but reading this nodejs/node#31710 the recommended approach seems to be to use URL.pathToFileURL https://nodejs.org/api/url.html#url_url_pathtofileurl_path.
There was a problem hiding this comment.
@richardsimko, how could i use it in loadModules method?
There was a problem hiding this comment.
I don't think you can do anything as a consumer, but you can submit a PR here to fix it :)
Fixes #210
Sadly it's not possible to test the native specific parts since jest doesn't do mocks of the native
import()function. However since I broke all the logic out to separate functions the real difference between loading modules using ES modules and CJS is negligible.