Reduce dependencies size

[kytta]

This PR reduces the dependency tree of the package by replacing some dependencies with more lightweight alternatives. Closes #978.

Changes made

• replace cosmiconfig with lilconfig
  • this also adds js-yaml since lilconfig doesn't provide YAML parsing

[kytta]

cc: @ai

I have tried to reduce the size of node_modules, but it didn't bring a lot:

Update: earlier I mentioned that the size went up. That's not true: I've just installed the local package incorrectly.

---

It turned out to be almost useless to try and remove such packages as chalk, cosmi-config, and others, since chalk still exists in listr2, replacements for which I couldn't (yet) find. Other packages (be it taskz, listr3, or listr) all implement chalk as well.

Another dependency hoard comes from execa, which is also semi-irreplaceable. One could, of course, use child_process.spawn() and lose all benefits of execa...

I could also go and patch listr2 and execa, like I did with log-symbols, yet I don't know if I'll be ready to maintain those patches.

What would be the best solution for this project?

[kytta]

P. S. I can't quite understand why some tests are failing. Apparently, log-symbols now uses Unicode (although it didn't).

What I can understand is that fixed-width-string introduces unwanted white spaces (i.e. pads strings), unlike cli-truncate

[iiroj]

Honestly, I think we are already using the most minimal amount of dependencies possible. Of course those dependencies might have a lot of extra cruft, but I don't think lint-staged directly has anything extra. I've tried to slim down unnecessary stuff and also keep them updated.

Tests might fail because log symbols differ based on platform, and tests might work around this by mocking or serializing them differently.

[okonet]

I also question this work unless there is something obvious. I think the easiest way to reduce the size is for all libraries to only ship a minimal required files but I don’t see how lint-staged itself can solve this. Also is 21 MB really a problem for a Dev dependency? To me this kind of work add little to no value but costs a lot of time.

[iiroj]

We could try a separate package.json for lint-staged's tests using yarn workspaces. Most of the dependency size is probably due to those, so maybe if they're separated into a subdirectory, it might reduce the main install size.

[okonet]

Hmm that’s interesting idea indeed. Is there a native npm behavior that could help us? Something like optional dependencies that are then installed on CI? I’m wondering why are dev dependencies are installed when ping-staged added as a dependency and if we could look into that somehow?

[kytta]

Nope, the dev dependencies aren't being installed when performing npm i lint-staged or similar. 20 MB are the production dependencies; dev dependencies take extra ~200 MB

[ai]

20 MB of dependencies is a lot. The problem is that most of them is package.json, README.md, CHANGELOG.md, tests.

Seems like the listr2 is a source of the problem. It is alone adds 18 MB to node_modules.

How hard it will be to create a tiny analog or reimplement it in lint-staged?

[ai]

Honestly, I think we are already using the most minimal amount of dependencies possible.

It is not is you will count sub-dependnecies. https://npm.anvaka.com/#/view/2d/lint-staged

There are 94 dependencies in lint-staged.

[iiroj]

listr2 has been very convenient. It basically handles all the cli graphics for us without us having to really think about. In addition to the display, we use it as a task runner. I'd say it's not feasible to try and reimplement it.

Maybe this effort could be focused directly on listr2? That would also benefit lint-stagedgreatly.

[ai]

Maybe this effort could be focused directly on listr2?

Yeap. Seems like it is better to send PR to listr2.

If we accept this PR (chalk→colorette) it will help it PR to listr2.

[iiroj]

If listr2 changes to colorette, we can follow the decision here no problem. Let's start that way, and we can link to discussion here. I believe lint-staged is a major user of listr2 so I'm sure we can come to an agreement on that.

[iiroj]

One major feature to consider is the supportsColor method, which we (try to) use to detect simpler shells, vs fancy terminals:

https://github.com/okonet/lint-staged/blob/4f9a146708862b06d19c7627f4dd1b094b2b88ce/bin/lint-staged.js#L8-L11

[kytta]

One major feature to consider is the supportsColor method, which we (try to) use to detect simpler shells, vs fancy terminals

supportsColor is actually a part of the supports-color package, which I included together with Colorette.

It is also to be noted, that Colorette, apparently, offers its own, albeit simpler implementation, where colorette.options.enabled signifies, whether colour is supported or not (without specifying the amount of colours)

[kytta]

I have posted a PR to listr2: listr2/listr2#408; let's see, what they say

[iiroj]

Nice work @NickKaramoff, seems like it got already merged.

Maybe if we reduce this PR to only target replacing chalk with colorette, and wait for an updated listr2 with the same change, we could get a nice result.

Let's handle the other dependencies in separate PR's.

[kytta]

I have reverted the removal of cli-truncate since the package I used instead, fixed-width-string, behaves differently.

I have also updated listr2, and the size of the package dropped to 15 MB, shaving off 5 MB 🎉
Apart from that, all the other packages indirectly remove Chalk (it can be found as a dependency in both cosmiconfig and log-symbols). I'd leave the PR as it is

[kytta]

Now, when it comes to testing, I find it bizarre that the AppVeyor builds are now broken. Apparently Jest expects the CLI o=to output non-Unicode characters from log-symbols (e.g. √ instead of ✔). Yet, I don't understand why that is expected and why it wasn't the case before. It's no matter whether one uses Sindre's log-symbols or mine — they both depend on is-unicode-supported.

Should we just update the Jest snapshots?

[ai]

Very strange that Unicode symbols were not used in CI. According to is-unicode-supported we should have Unicode symbols before.

I found that in main branch log-symbols was not even called during tests (just add throw new Error to log-symbols). Seems like something mock log-symbols.

(I also think that we should not use log-symbols dependencies at all. It is so simple that it is better to move it to utils/messages. Docs + package.json for these dependencies is bigger than their code.)

[iiroj]

I believe we use log-symbols because listr2's tasks also use it to display status of the tasks, and we want to show additional messages with the same symbols.

it might be that the Appveyor terminal doesn't suppor colors and so they're not used "on Windows"... what about the tests running on GitHub CI?

[ai]

it might be that the Appveyor terminal doesn't suppor colors and so they're not used "on Windows"... what about the tests running on GitHub CI?

The same. It is not connected with CI.

If you run Jest locally log-symbol/index.js will not be called too (but it was called when you execute lint-staged binary manually). There is a mock somewhere, which blocks log-symbols/index.js for Jest environment.

[ai]

I believe we use log-symbols because listr2's tasks also use it to display status of the tasks, and we want to show additional messages with the same symbols.

listr2 doesn’t use log-symbols. It uses is-unicode-supported and figures

[iiroj]

Thanks for the clarification, it has been some time since I've touched these.

This should be the auto mock file:
https://github.com/okonet/lint-staged/blob/master/test/__mocks__/log-symbols.js

I think it would be preferable to use the same packages as listr2 instead of inlining code, since that would reduce duplication, yes?

[kytta]

This should be the auto mock file:
https://github.com/okonet/lint-staged/blob/master/test/mocks/log-symbols.js

OK, I see. I'll figure out how to fix it and update the PR 👌

I think it would be preferable to use the same packages as listr2 instead of inlining code, since that would reduce duplication, yes?

Theoretically yes, but it would save even more space if both listr2 and lint-staged would inline this code. The changes to the former were proposed (listr2/listr2#412) and will probably be accepted today

[iiroj]

@NickKaramoff it would perfectly fit us if we could import and use those same symbols directly from listr2, since the idea is that listr2 has a nice UI and we just want show additional messages that look similar.

Something like const { symbols } = require('listr2')

[kytta]

it would perfectly fit us if we could import and use those same symbols directly from listr2

Implemented this exactly in this way 🙌

---

I've tried my best to fix the CI. Some stuff (like extra white spaces) were existent before this PR (at least on my machine) and some new stuff was introduced, which has to do something with Git... I honestly do not understand this happens 😅

[iiroj]

@NickKaramoff Honestly, I don't have any idea either. I've also had a lot of trouble trying to figure out why (especially) Appveyor tests behave like they do... It would be nice to ditch that and use GitHub Actions for all platforms.

Now one thing that seems directly related in the test snapshots is this:
https://github.com/okonet/lint-staged/pull/981/checks?check_run_id=2705272685#step:10:812

The new line has a space at the end, while old doesn't:

---    - ERROR
+++    + ERROR 

[kytta]

The new line has a space at the end, while old doesn't:

---    - ERROR
+++    + ERROR 

@iiroj sorry that it took to long to reply. This issue is weird, since this particular line of output you've linked to is being printed out by logger.error() in lib/index.js; I don't see any plausible reason for it to print a space 🤔

[kytta]

Found it! It's consolemock's fault

Apparently, when using a console method without any arguments, it adds an extra space to the output, which, I guess, it didn't do before?

[iiroj]

This PR #1003 merged an updated Listr2, which recently had a cleanup of its own dependencies. I also re-used the figures exported by it.

[kytta]

I have come back and rebased the changes made independently of this PR. Now it only replaces cosmiconfig with lilconfig, the rest was done in other PRs

On a good note: I discovered the error with the tests. I didn't notice that I myself committed the wrong snapshots. This happened because of EditorConfig automatically removing trailing whitespace 😅 Now the tests should run smoothly

[codecov]

Codecov Report

Merging #981 (d9274a8) into master (f861d8d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #981   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          654       656    +2     
  Branches       149       149           
=========================================
+ Hits           654       656    +2     

Impacted Files	Coverage Δ
lib/index.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f861d8d...d9274a8. Read the comment docs.

[iiroj]

Let's merge this one. If there's other ways to reduce dependency size, we should open a separate PR for each individual change! 🚀

[github-actions]

🎉 This PR is included in version 11.2.4 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 11.3.0-beta.2 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀