Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic import of TS files from inside node_modules #1939

Closed
6 tasks done
B4nan opened this issue Aug 30, 2022 · 23 comments · Fixed by #1944
Closed
6 tasks done

Dynamic import of TS files from inside node_modules #1939

B4nan opened this issue Aug 30, 2022 · 23 comments · Fixed by #1944

Comments

@B4nan
Copy link

B4nan commented Aug 30, 2022

Describe the bug

If I do await import('./user.ts') from inside a test, it works just fine. If I move this dynamic import into a dependency, it fails with following:

TypeError: Unknown file extension ".ts" for ./user.ts
      1| export async function importTest() {
      2|     return await import('./user.ts');
       |            ^
      3| }
      4| 

In actual test i am using absolute paths to be sure, simplifying here for readability. The contents of the file as simplified for testing to just export const User: any = {}.

I even tried to create a simple pkg folder with package.json and a single file exporting that function. Works fine if its outside of node_modules, once I move it there, it fails this way. Doesnt matter if I use import from 'pkg' or import from '../node_modules/pkg/index.mjs'.

Tried to enable registerNodeLoader, that changes the error to following:

SyntaxError: Missing initializer in const declaration

This is because it is TS file, and it fails on the first TS part. If I remove the type hint (which makes it a valid JS file), it works fine.

Then I finally tried inline: ['pkg'] and it started working. But this workaround does not work for me in the actual use case, where it breaks the dependency (@mikro-orm/core).

Reproduction

  1. Create user.ts file with just export const AuthorListing: any = {}.
  2. Create node_modules/pkg folder with index.mjs file:
export async function importTest(file) {
    return await import(file);
}
  1. Run importTest('./user.ts') from a test file.

System Info

System:
    OS: macOS 12.5.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 97.61 MB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.8.0 - ~/Library/Caches/fnm_multishells/98665_1661811231718/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 8.18.0 - ~/Library/Caches/fnm_multishells/98665_1661811231718/bin/npm
  Browsers:
    Chrome: 104.0.5112.101
    Firefox: 103.0.1
    Safari: 15.6.1

Used Package Manager

yarn

Validations

@sheremet-va
Copy link
Member

deps.inline should just work. This is not "a workaround", this is a solution for applying Vite transformations to files outside of your source code. You can use regular expressions, if it doesn't find your package. Your reproduction also doesn't do anything for me - if you still have problems with inline, try creating an actual runnable reproduction using stackblitz or create a repo in GitHub with code itself.

@github-actions
Copy link

Hello @B4nan. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with need reproduction will be closed if they have no activity within 3 days.

@B4nan
Copy link
Author

B4nan commented Aug 30, 2022

Thanks for the prompt response!

deps.inline should just work.

When I put there the ORM core package, I get this error:

 ❯ node_modules/@mikro-orm/core/dist/index.mjs:4:36
TypeError: Cannot read properties of undefined (reading 'ARRAY_OPERATORS')
      2| 
      3| export default mod;
      4| export const ARRAY_OPERATORS = mod.ARRAY_OPERATORS;
       |                                    ^
      5| export const AbstractNamingStrategy = mod.AbstractNamingStrategy;
      6| export const AbstractSchemaGenerator = mod.AbstractSchemaGenerator;
 ❯ async /Users/adamek/htdocs/mikro-orm/tutorial/src/app.ts:5:31
 ❯ async /Users/adamek/htdocs/mikro-orm/tutorial/test/utils.ts:1:256
 ❯ async /Users/adamek/htdocs/mikro-orm/tutorial/test/user.test.ts:3:31

(the file is here: https://unpkg.com/browse/@mikro-orm/core@5.3.1/index.mjs)

The package provides hybrid ESM support for named imports via gen-esm-wrapper. Maybe it inlined only index.mjs and ignored index.js where the implementation actually is? And can I inline only a single file of a package? That would do it for me too, still for the purpose of the tutorial I am working on, this seems like quite a hack, better to add the whole package there if possible, than depending on its implementation details that can change.

this is a solution for applying Vite transformations to files outside of your source code.

I get that, but the transformations should happen on a local file, not inside that package, so I was kinda expecting this to work. And then there is the need for registerNodeLoader otherwise it does not understand the TS extension, is that also intended. The package only has dynamic import in the code, the file that is imported is local.

Feels a bit weird that the exact same import behaves differently inside node_modules (using absolute path).

try creating an actual runnable reproduction using stackblitz or create a repo in GitHub with code itself.

I tried to go with stackblitz but for that I need to move things to node_modules, and I dont think I can do that there. Will you be ok with a repo that moves the folder to node_modules in a postinstall hook or something like that? I know its quirky, but this seems to be issue with dependencies, and I dont want to publish some junk because of reproducing things...

Or maybe it wont be needed if you say this is expected behaviour :] Will provide something tomorrow if needed.

@sheremet-va
Copy link
Member

sheremet-va commented Aug 30, 2022

And then there is the need for registerNodeLoader otherwise it does not understand the TS extension

registerNodeLoader only transforms import paths, it doesn't do anything with code itself.

Will you be ok with a repo that moves the folder to node_modules in a postinstall hook or something like that?

It will be ok.

When I put there the ORM core package, I get this error

What exactly is the problem? It doesn't understand typescript or mikro imports? If you need to transform files imported from some_file.ts, you need to inline that file (some_file.ts) manually: Vitest transforms all import calls to __vite_import__, and applies its own resolution, so if import is not transformed to this magical function, it will be imported by native Node, and will fails, unless you have some kind of --loader.

@sheremet-va
Copy link
Member

sheremet-va commented Aug 30, 2022

When I put there the ORM core package, I get this error:

Looking at your generated code, I can say that Node doesn't see named imports because they are applied dynamically (I think). You need to use mod.default to access those inside ESM. See: https://github.com/nodejs/cjs-module-lexer/tree/1.2.2

@B4nan
Copy link
Author

B4nan commented Aug 30, 2022

Hmm but that index.mjs file works just fine, only once it gets inlined it fails this way.

@sheremet-va
Copy link
Member

sheremet-va commented Aug 30, 2022

Hmm but that index.mjs file works just fine, only once it gets inlined it fails this way.

Interesting. Please, create a reproduction with this case, I will look into it.

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

Here is the repro, its really as simple as this:

https://stackblitz.com/edit/vitest-dev-vitest-d33pe8?file=test/basic.test.ts

Same code works fine when executed directly, not via vitest. I do have a working ESM project example here (not TS, but thats unrelated).

@sheremet-va
Copy link
Member

Here is the repro, its really as simple as this:

stackblitz.com/edit/vitest-dev-vitest-d33pe8?file=test/basic.test.ts

Same code works fine when executed directly, not via vitest. I do have a working ESM project example here (not TS, but thats unrelated).

Ok, if it works without inlining what is the problem?

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

Ok, if it works without inlining what is the problem?

You said the correct solution to the original problem is inlining. This is the module where dynamic import happens.

@sheremet-va
Copy link
Member

Ok, if it works without inlining what is the problem?

You said the correct solution to the original problem is inlining. This is the module where dynamic import happens.

This has nothing to do with TypeScript and your original problem though. The problem here is that Vitest doesn't also store all commonjs properties on default key, which is a bug, you can bypass it by using config

{
  deps: {
    inline: [/mikro\/core\/index.mjs/],
    external: [/mikro\/core\/index.js/],
  }
}

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

While this fixes the issue with inlining, it doesnt seem to work for the original use case. I still see the same errors as in the OP.

Updated the repro to showcase the actual problem, where I do the dynamic import via MikroORM.

https://stackblitz.com/edit/vitest-dev-vitest-d33pe8?file=user.ts,test%2Fbasic.test.ts,vite.config.ts,package.json

Currently it works via eval trick (Function(return import('${id}'))()) to keep the dynamc import instead of compiling to require. I tried to change this to actual import locally, that itself didnt help, but in this repro I can see it behaves differently too. I think once the code is inside node_modules, it fails the same in both ways.

https://unpkg.com/browse/@mikro-orm/core@5.3.2-dev.32/dist/utils/Utils.js#L738

(note: this is using latest dev version, dev versions are published with the dist folder, so I adjusted the paths based on that)

@sheremet-va
Copy link
Member

it doesnt seem to work for the original use case

This is why I was confused earlier.

Currently it works via eval trick

Well, Vitest doesn't support this usage, unfortunately. We need to replace it, as I said before, otherwise it can't go through Vite pipeline.

I tried to change this to actual import locally, that itself didnt help, but in this repro I can see it behaves differently too

I don't understand what you mean here 😄 Does it work, or does it not? I can see that you can import user.ts in your reproduction.

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

What I mean is that I literally changed this eval trick with a real import call locally. And it does not help.

So I understand it will need to be changed on my end to support it, but I tried already and it still fails the same.

@sheremet-va
Copy link
Member

What I mean is that I literally changed this eval trick with a real import call locally. And it does not help.

So I understand it will need to be changed on my end to support it, but I tried already and it still fails the same.

Can you give a proper reproduction where Vitest fails for you? You are saying that "when you do something", "something works/doesn't work" - I can't really help, when I don't see code. All previous cases are discussed and explained. If I "change" import to actual import call, I get "No entities were discovered" error, for example.

@sheremet-va
Copy link
Member

By the way, even if you could import TypeScript file, I'm not sure if Vitest can properly transform it, since esbuild doesn't support decorator metadata (I don't know if you use it, but I see you are using decorators): #708 (comment)

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

Let me quickly fix the imports and publish new dev version so we can forget about this problem. Btw I dont use reflect-metadata, so no problem with decorators (its pretty much unusable with ESM as it fails to resolve cycles and errors on runtime because of it).

If you dont mind, I could invite you to the private repo where I have the full project. It is a tutorial I am working on, so nothing complex. There you could see the app that runs fine (compiled via node as well as TS via ts-node-esm, but same code fails with vitest). I was trying to isolate this, but I see it's not really helpful and only adds confusion (so sorry about that).

@sheremet-va
Copy link
Member

If you dont mind, I could invite you to the private repo where I have the full project. It is a tutorial I am working on, so nothing complex. There you could see the app that runs fine (compiled via node as well as TS via ts-node-esm, but same code fails with vitest). I was trying to isolate this, but I see it's not really helpful and only adds confusion (so sorry about that).

Sure, I am open to it.

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

@sheremet-va
Copy link
Member

sheremet-va commented Aug 31, 2022

So now in version dev-33 it is a real dynamic import:

unpkg.com/browse/@mikro-orm/core@5.3.2-dev.33/dist/utils/Utils.js#L712

And in that stack blitz it fails the same.

stackblitz.com/edit/vitest-dev-vitest-d33pe8?file=user.ts,test%2Fbasic.test.ts,vite.config.ts

Utils is loaded with require, so it doesn't go through Vite pipeline, since it's asynchronous. Vitest processes only files that are loaded with import.

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

Hmm I see, so this can only work with real ESM projects, and this get-esm-wrapper wont be enough? But if this function which does the dynamic import would be defined directly in the index.mjs file, that might help I guess? Will try to refactor it that way.

edit: or it would be still a problem given this function would be called through some other requires anyway? because it will still be triggered from inside some CJS file in the end

@sheremet-va
Copy link
Member

Hmm I see, so this can only work with real ESM projects, and this get-esm-wrapper wont be enough? But if this function which does the dynamic import would be defined directly in the index.mjs file, that might help I guess? Will try to refactor it that way.

edit: or it would be still a problem given this function would be called through some other requires anyway? because it will still be triggered from inside some CJS file in the end

If the file where await import is located is loaded with require, then it won't be affected by transformations. All imports in chain should be using import. All require calls are delegated to Node.

@B4nan
Copy link
Author

B4nan commented Aug 31, 2022

Understood. I went with a different approach, using a global variable with a dynamic import callback, which can be overridden by user via Utils.setDynamicImportProvider(id => import(id)), so the actual code that does the import is from userland, seems like this way it works fine. Its a quirk, but at least something :] (edit: in the end I dont need any vitest config so its just a one line I will need to describe)

Thanks a lot for your insights.

@sheremet-va sheremet-va closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants