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

Optional dependency on ts-node #3221

Closed
1 of 4 tasks
matthieusieben opened this issue Jun 3, 2022 · 9 comments
Closed
1 of 4 tasks

Optional dependency on ts-node #3221

matthieusieben opened this issue Jun 3, 2022 · 9 comments
Labels

Comments

@matthieusieben
Copy link

Expected Behavior

ts-node should be an optional dependency of the project

Current Behavior

currently, installing @commitlint/cli results in a lot of downloaded NPM packages.

Affected packages

  • cli
  • core
  • prompt
  • config-angular

Possible Solution

Instead of statically importing cosmiconfig-typescript-loader, @commitlint/load could depend on on it using an optional peer dependency and dynamically require / import() that dependency if/when a .ts file is detected, and show a warning if that fails:

Using TypeScript @commitlint configurations requires `@commitlint/ts` to be installed as a peer dependency of your project

Steps to Reproduce (for bugs)

Not really a bug.

$ yarn install @commitlint/cli

Context

I don't use Typescript. So I really don't see why ts-node should be a mandatory dependency of my project:

$ y why ts-node

yarn why v1.22.18
[1/4] 🤔  Why do we have the module "ts-node"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "_project_#@commitlint#cli#@commitlint#load#cosmiconfig-typescript-loader" depends on it
   - Hoisted from "_project_#@commitlint#cli#@commitlint#load#cosmiconfig-typescript-loader#ts-node"
info Disk size without dependencies: "1.5MB"
info Disk size with unique dependencies: "2.97MB"
info Disk size with transitive dependencies: "3.25MB"
info Number of shared dependencies: 16

Your Environment

Executable Version
commitlint --version @commitlint/[email protected]
git --version git version 2.36.1
node --version v16.15.0
@escapedcat
Copy link
Member

escapedcat commented Jun 3, 2022

Relates to #3218 I guess.
Happy for a PR if you're motivated and have time.

Had a look where this was introduced: #3007 (comment)

So looks like we wanted to avoid a breaking change.
Happy for a PR which is cleaning this up and create a major version.
Maybe @jrolfs has some feedback to this as well.

@jrolfs
Copy link
Contributor

jrolfs commented Jun 10, 2022

This absolutely makes sense to me. I think it might be a little tricky to implement, though, because isn't the point of Cosmic Config to delegate configuration discovery (which files etc.)? I guess, even as someone who uses TypeScript almost exclusively, I'd also question the value of supporting TypeScript-based configuration files. I think the easiest move here might be to drop support for those, but I don't know how popular that feature is.

@jrolfs
Copy link
Contributor

jrolfs commented Jun 10, 2022

Hrm, maybe we could wrap the loader in a function that ensures the optional dependency:

import { cosmiconfig } from "cosmiconfig";
import TypeScriptLoader from "cosmiconfig-typescript-loader";

const moduleName = "module";
const explorer = cosmiconfig("test", {
  searchPlaces: [
	// ...
  ],
  loaders: {
    ".ts": TypeScriptLoader(), // ← wrap this in a function that loads `cosmiconfig-typescript-loader` dynamically
  },
});

const cfg = explorer.load("./");

@escapedcat
Copy link
Member

escapedcat commented Jun 11, 2022

Thanks for your feedback @jrolfs !
There were quite some people involved to add the TS support. I believe we should support this because this is were projects are heading in the future.

Not sure how many people use a TS config. But also not sure how many people care about the size of commitlint really (Relates to #3040, #1791).
I'll let this rest a bit and might get back to it at a later point.

@simonecorsi
Copy link

But also not sure how many people care about the size

I just wanted to advocate for the people who care about the size since we're heavy users of your libs (btw thx for it 🙏).

The hard-dep makes our docker image nearly ~100MB bigger, which may seem not much nowadays, but equates to more bandwidth usage which is costly, and cumulative time wasted by developers waiting image pulls in CI/locally. This is just a matter of DevX to me :)

@escapedcat
Copy link
Member

Reopening because of #3641
This will then hopefully be fixed with a new mayor version.
yolo

@escapedcat escapedcat reopened this Aug 10, 2023
@binomialstew
Copy link

binomialstew commented Sep 14, 2023

I'm also seeing commitlint fail in our project with Error: Cannot find module 'typescript' as of v17.7.1.
My mistake--this was caused by another dependency in our package, cz-conventional-changelog, requiring an earlier version of @commitlint/cli.

@elmpp
Copy link

elmpp commented Oct 1, 2023

This is even more necessary given the rise of bun.

oven-sh/bun#6184

@escapedcat
Copy link
Member

Solved by #3722

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

6 participants