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

Lazy loading for ts-node + optional peer-dependecies #79

Closed
thekip opened this issue Feb 6, 2023 · 4 comments · Fixed by #106
Closed

Lazy loading for ts-node + optional peer-dependecies #79

thekip opened this issue Feb 6, 2023 · 4 comments · Fixed by #106
Labels
enhancement New feature or request

Comments

@thekip
Copy link

thekip commented Feb 6, 2023

Hey, thanks for the job.

Currently, loader implemented in such a way that it doesn't matter does the cosmiconfig found a .ts file or not ts-node is trying to load.
The cosmiconfg + cosmiconfig-typescript-loader is usually used on the projects (libraries) where authors want to give users some flexibility of config files. It means some part of users want to use .rc files with json content, another .js and some .ts.

But once you add this loader to the project, ts-node become a required dependency even if you don't use typescript configs. And application would break if you will not install ts-nodem which might be not necessary for pure js projects.

Related issue: lingui/js-lingui#1401

Currently, i implemented a workaround, which lazy-load a cosmiconfig-typescript-loader only if a *.ts file is trying to be loaded. But I think this logic could be implemented in loader itself.

Also, that would be great to mention in the readme, for library authors. If they are going to use it in theirs project, they have to add all peer-depedencies from this loader to the upstream project. (as optional, if lazy-loading would be implemented)

@Codex-
Copy link
Owner

Codex- commented Feb 6, 2023

Currently, ts-node is a required peer dependency, as it's expected that anyone consuming this is intending to use this loader or otherwise not include it as a dependency. This begs the question, why would you depend on this package if you weren't intending to load ts configs in cosmiconfig? 🤔

The lingui package @lingui/conf has this as a hard dependency, but has noted that it is an 'internal' package, then the packages @lingui/babel-plugin-extract-messages, @lingui/cli, @lingui/loader, @lingui/macro, @lingui/snowpack-plugin, and @lingui/vite-plugin depend on @lingui/conf as a hard dependency. Based on this, it seems appropriate that at the conf package level this loader is lazy loaded depending on the usage by consumers.

Happy to take a look this week at introducing lazy loading, or happy to review PR's if it's urgent 😄

I imagine something like:

export function TypeScriptLoader(options?: RegisterOptions): Loader {
  let tsNodeInstance: Service;
  return (path: string, content: string) => {
    try {
      if (!tsNodeInstance) {
        const { register } = require("ts-node") as typeof import("ts-node");
        tsNodeInstance = register({
          ...options,
          compilerOptions: { module: "commonjs" },
        });
      }
...

Would suffice, however would need to explore testing, since vitest is esm first I'd probably have to go back to jest to have effective testing on this

@thekip
Copy link
Author

thekip commented Feb 6, 2023

The implementation proposed is correct. This should work. Regarding ESM - it's a hard topic.
To make it lazy load with ESM it should be loaded asynchronously. But many of the projects out there, including lingui are not ready (and probably will not be ready) to read config asynchronously.

So i think using Vitest with esm-first support is a good thing, but for the old project such as cosmiconfig probably not a good choice for now.

@Codex-
Copy link
Owner

Codex- commented Mar 1, 2023

@thekip apologies for the delay, can you please review #84 when you get a chance and we can get this resolved :)

@Codex- Codex- added the enhancement New feature or request label Mar 21, 2023
@mdrodz
Copy link

mdrodz commented Apr 3, 2023

Additionally, you should make the peer dependency optional in package.json, no?

"peerDependenciesMeta": {
  "ts-node": {
    "optional": true
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants