Codex- / cosmiconfig-typescript-loader

TypeScript config file handler for cosmiconfig
MIT License
30 stars 7 forks source link

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

Closed timofei-iatsenko closed 1 year ago

timofei-iatsenko commented 1 year ago

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: https://github.com/lingui/js-lingui/issues/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- commented 1 year ago

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

timofei-iatsenko commented 1 year ago

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- commented 1 year ago

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

mdrodz commented 1 year ago

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

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