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

Reduce dependencies size #981

Merged
merged 1 commit into from Oct 23, 2021
Merged

Reduce dependencies size #981

merged 1 commit into from Oct 23, 2021

Conversation

@NickKaramoff
Copy link
Contributor

@NickKaramoff NickKaramoff commented May 24, 2021

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
@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 24, 2021

cc: @ai

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

Before: 21M

After: 20M

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?

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 24, 2021

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

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 24, 2021

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.

Loading

@okonet
Copy link
Owner

@okonet okonet commented May 24, 2021

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.

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 24, 2021

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.

Loading

@okonet
Copy link
Owner

@okonet okonet commented May 24, 2021

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?

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 24, 2021

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

Loading

@ai
Copy link
Contributor

@ai ai commented May 24, 2021

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?

Loading

@ai
Copy link
Contributor

@ai ai commented May 24, 2021

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.

20210524092559

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 24, 2021

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.

Loading

@ai
Copy link
Contributor

@ai ai commented May 24, 2021

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 (chalkcolorette) it will help it PR to listr2.

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 24, 2021

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.

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 24, 2021

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

const { supportsColor } = require('chalk')
if (supportsColor && supportsColor.level) {
process.env.FORCE_COLOR = supportsColor.level.toString()
}

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 24, 2021

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)

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 24, 2021

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

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 24, 2021

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.

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 24, 2021

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

Loading

@NickKaramoff NickKaramoff marked this pull request as ready for review May 24, 2021
@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 24, 2021

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?

Loading

@ai
Copy link
Contributor

@ai ai commented May 24, 2021

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.)

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 25, 2021

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?

Loading

@ai
Copy link
Contributor

@ai ai commented May 25, 2021

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.

Loading

@ai
Copy link
Contributor

@ai ai commented May 25, 2021

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

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 26, 2021

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?

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 26, 2021

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 (cenk1cenk2/listr2#412) and will probably be accepted today

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 26, 2021

@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')

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented May 30, 2021

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 😅

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented May 30, 2021

@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 

Loading

lib/index.js Outdated Show resolved Hide resolved
Loading
@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented Jun 9, 2021

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 🤔

Loading

@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented Jun 9, 2021

Found it! It's consolemock's fault

grafik

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?

Loading

@iiroj
Copy link
Collaborator

@iiroj iiroj commented Oct 4, 2021

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.

Loading

Replace 'cosmiconfig' with 'lilconfig' due to the smaller size and
dependency tree of the latter package. This also installs 'js-yaml',
since 'lilconfig' doesn't provide YAML parsing.
@NickKaramoff
Copy link
Contributor Author

@NickKaramoff NickKaramoff commented Oct 21, 2021

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

Loading

@NickKaramoff NickKaramoff requested a review from iiroj Oct 21, 2021
@codecov
Copy link

@codecov codecov bot commented Oct 21, 2021

Codecov Report

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

Impacted file tree graph

@@            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.

Loading

lib/index.js Show resolved Hide resolved
Loading
iiroj
iiroj approved these changes Oct 23, 2021
Copy link
Collaborator

@iiroj iiroj left a comment

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

Loading

@iiroj iiroj merged commit 04529e2 into okonet:master Oct 23, 2021
11 checks passed
Loading
@github-actions
Copy link

@github-actions github-actions bot commented Oct 23, 2021

🎉 This PR is included in version 11.2.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Loading

@iiroj iiroj linked an issue that may be closed by this pull request Oct 24, 2021
iiroj added a commit that referenced this issue Oct 26, 2021
iiroj added a commit that referenced this issue Oct 26, 2021
* Revert "fix: correctly import `js-yaml` to fix yaml config loading (#1033)"

This reverts commit 612d806.

* Revert "perf: replace `cosmiconfig` with `lilconfig` (#981)"

This reverts commit 04529e2.
@github-actions
Copy link

@github-actions github-actions bot commented Oct 30, 2021

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

The release is available on:

Your semantic-release bot 📦🚀

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants