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

Refactor to replace cosmiconfig with lilconfig and js-yaml #6101

Closed
binyamin opened this issue May 19, 2022 · 28 comments · Fixed by #6491
Closed

Refactor to replace cosmiconfig with lilconfig and js-yaml #6101

binyamin opened this issue May 19, 2022 · 28 comments · Fixed by #6491
Labels
status: needs discussion triage needs further discussion

Comments

@binyamin
Copy link
Contributor

binyamin commented May 19, 2022

Note: I got all the bundle sizes from Bundlephobia

Problem

Stylelint takes up around 378kb when minified. That's quite large for a linter.

Details

Nearly a quarter of the size comes from the yaml parser, yaml, which is 91.9kb minified. An alternative called js-yaml is about half the size (38.8kb minified).

The yaml parser is a dependency of cosmiconfig (138.4kb minified). lilconfig is smaller (4kb minified), and mostly matches the cosmiconfig API. The main exception is yaml parsing, which can be easily added.

Solution

Reduce bundle size dramatically, by replacing cosmiconfig with a combination of lilconfig and js-yaml.

@ybiquitous
Copy link
Member

ybiquitous commented May 20, 2022

@binyamin Thanks for the proposal. It sounds good to me. 👍🏼

Do you know details about compatibility between cosmiconfig and lilconfig?

Our documentation has mentioned cosmiconfig, so this change may make breaking changes.

Stylelint uses cosmiconfig to find and load your configuration object.

https://stylelint.io/user-guide/configure

@ybiquitous ybiquitous added the status: needs discussion triage needs further discussion label May 20, 2022
@ybiquitous
Copy link
Member

@binyamin
Copy link
Contributor Author

@ybiquitous According to the readme, it has the same API. The key difference here seems to be the lack of yaml parsing, but you can add custom loaders.

Lilconfig does not intend to be 100% compatible with cosmiconfig but tries to mimic it where possible. The key differences are:

  • no support for yaml files out of the box(lilconfig attempts to parse files with no extension as JSON instead of YAML). You can still add the support for YAML files by providing a loader, see an example below.
  • no cache

(ref)

@ybiquitous
Copy link
Member

@binyamin Thank you.

If not 100% compatible, it seems reasonable to me that we would include this change in the next major version (15.0.0).

Any thoughts?

@binyamin
Copy link
Contributor Author

@ybiquitous This is a large project, so that makes sense.

We could also test it, once it's implemented. It's conceivable that there won't be any breaking changes, since any end-user functionality can be added. Cosmiconfig enables caching by default, so that may impact performance for repeated calls to stylelint.

I'll see if I have time to implement this over the coming week.

@JounQin
Copy link
Member

JounQin commented May 25, 2022

The js-yaml package doesn't strictly follow the YAML 1.2 spec. It supports some YAML 1.1 features.

unifiedjs/unified-engine#65

@binyamin
Copy link
Contributor Author

Note: Apologies for the wall of text. I got carried away, looking at the test suites 😅

@JounQin Interesting. I see the merits in using a fully-compliant parser. I'm not sure I understand that issue, though.

According to this matrix of YAML 1.2 test results, most parsers (including yaml) don't fully support the YAML 1.2 spec1. Also, in many of the areas where js-yaml fails, it's trying to handle some special syntax that I don't see very often.

Syntax Example
# anchor
- &a "value"
# alias
- *a
# tag
- !foo
- !!bar

Ideally, stylelint would use a fully-compliant YAML parser for the config file. However, in the grand scheme of things, I think using js-yaml is relatively fine.

  1. Not all users will need a config file. People can use the package.json, the JS API, a build tool, or the default settings.
  2. Not all config files use YAML. It might use JS or JSON.
  3. Not all YAML configs need yaml. Most language features will work regardless.
  4. Note that yaml has a much larger bundle size than js-yaml. See the top comment.

In my personal opinion, it seems only fair to offer a smaller bundle size, since it won't affect the majority of users. Ultimately, though, it's not my call (cc @ybiquitous ).

Footnotes

  1. Ironically, both "official" parsers, PyYAML and LibYAML, only pass about 82% of the tests.

@ntwb
Copy link
Member

ntwb commented May 26, 2022

Just some historical context of cosmiconfig for reference, @davidtheclark is a co-owner of the Stylelint GitHub org and wrote cosmiconfig originally for Stylelint, see #490 & #503

Any changes to how Stylelint loads configurations will have to be very carefully considered:

  • there's 610 shared configs published to npm here...
  • there's 4,023 stylelintrc YAML files on GitHub here...

I wonder if it's worth exploring using js-yaml instead of yaml in cosmiconfig to potentially acheive the same performance benefit without switching libraries

@ybiquitous
Copy link
Member

@ntwb Thanks for sharing the historical context.

cosmiconfg v6 replaced js-yaml with yaml for some reason:

I'm not sure the replacement reason, but there were some security issues for js-yaml.
See also https://github.com/davidtheclark/cosmiconfig/issues?q=is%3Aissue+%22js-yaml%22

@ntwb
Copy link
Member

ntwb commented May 26, 2022

Good catch @ybiquitous, it appears to be from security issues:

Via cosmiconfig/cosmiconfig#216 (comment)

I haven't had much time recently but changing js-yaml to yaml is the only thing that comes to mind (to avoid potential future security issues). I can probably get around to changing that dependency this weekend.

Seems the js-yaml package has settled since the above advisories, with no new advisories for the past ~3 years

https://snyk.io/vuln/npm%3Ajs-yaml

image

@binyamin
Copy link
Contributor Author

binyamin commented May 26, 2022

I wonder if it's worth exploring using js-yaml instead of yaml in cosmiconfig to potentially acheive the same performance benefit without switching libraries

I like this idea, but I'm worried that it might take a while for the PR to be merged.

For example, cosmiconfig/cosmiconfig#266 seems super-easy to fix. It's 5 months old, and there's been no activity. The maintainer himself doesn't seem to have been very active on GitHub recently.

To clarify: I have nothing against the maintainer. I simply want to do what's effective for this project. I do think that he should know, as a core stylelint member, that we're thinking about removing cosmiconfig as a dependency.

@ybiquitous
Copy link
Member

@binyamin @ntwb Thanks for clarifying the discussion.

It sounds better to me to continue the maintenance of cosmiconfg, instead of switching to another library.

I wish @davidtheclark would give someone (who has a passion for the maintenance of cosmiconfg) access to maintain and publish. If possible, it may be desirable to transfer the package repository to the github.com/stylelint organization.

This is just one of my opinions, though.

@davidtheclark
Copy link
Contributor

davidtheclark commented May 27, 2022

👋 Cosmiconfig is a stable project, and I've been a little restrictive about the features in order to keep it that way. I am happy to merge PRs that adjust dependencies. And I have no attachment to any particular yaml library -- just trying to let users solve their problems with security warnings. Happy to merge and deploy a dependency change.

I'm also happy to give other people access to maintain and publish, and have done so in the past. (If anybody here would like to be a cosmiconfig maintainer, sounds good to me.)

I will still not be interested in changing the package rapidly, pushing inconsequential complexities (e.g. native ESM), or pushing new defaults -- because I don't think the package needs those things and people are free to make other packages with different priorities. It's also true that I'm not interested in spending much personal time on open source projects right now.

That's the state of things!

@ybiquitous
Copy link
Member

@davidtheclark Good news! 🙌🏼

What do you think about updating the yaml dependency to v2 as @binyamin commented on #6101 (comment)?

@davidtheclark
Copy link
Contributor

davidtheclark commented May 27, 2022

See cosmiconfig/cosmiconfig#274: I prepped a v8 that switches back to js-yaml. I think there should be no effect on users. Please comment there if you have any concerns! I'll release tonight or later this weekend.

@davidtheclark
Copy link
Contributor

Upgrading yaml means removing support for Node 10 and 12. Switching to js-yaml means only removing support for Node 10, but might have some parsing differences on some obscure, infrequently used YAML syntax. Either way, it would be a major version bump for cosmiconfig.

What do Stylelint folks prefer? Stick with yaml, or switch to js-yaml as suggested in this issue?

If possible, it may be desirable to transfer the package repository to the github.com/stylelint organization

Also, if you're interested in pulling cosmiconfig into the Stylelint org and taking over, I'd be very happy to make that happen. What do you think?

@ybiquitous
Copy link
Member

What do Stylelint folks prefer? Stick with yaml, or switch to js-yaml as suggested in this issue?

I think Stylelint cannot bump to the major version immediately (e.g. dropping the Node.js 12 support or removing deprecated rules), so it seems better to me to switch to js-yaml instead of updating yaml.

Also, if you're interested in pulling cosmiconfig into the Stylelint org and taking over, I'd be very happy to make that happen. What do you think?

Thanks for the proposal, @davidtheclark. I believe this is an excellent suggestion and it should be a unique opportunity to touch a famous and widely-used package of cosmiconfig. 🙌🏼

What is your opinion on these above?

@ntwb
Copy link
Member

ntwb commented Jun 2, 2022

If possible, it may be desirable to transfer the package repository to the github.com/stylelint organization.

...in pulling cosmiconfig into the Stylelint org and taking over

I've no strong opinions on this, as long as if and when required new cosmiconfig releases can happen timely, be that adding additional contributors to the current repo, or moving the repo here

Cosmiconfig is a stable project

100%, if it isn't broken don't fix it, Cosmiconfig is in (or near) the top 100 packages downloaded per month, so regardless of anything else this is very important to keep it this way.

What do Stylelint folks prefer? Stick with yaml, or switch to js-yaml as suggested in this issue?

If this will have (for most users) limited impact, then per this:

Stylelint takes up around 378kb when minified. That's quite large for a linter.
Nearly a quarter of the size comes from the yaml parser, yaml, which is 91.9kb minified. An alternative called js-yaml is about half the size (38.8kb minified).

This would reduce Stylelint down to ~300kb, so that's very roughly a ~25% reduction in package size, so that's nice

So, overall happy to go with this subject to any further pending comments, considering this will be a major bump for Cosmiconfig and for Stylelint to drop Node.js v12 support I think we have a little time available to leave this issue sit here open for a little while longer to gather any additional feedback, and in particular I'd like to here @jeddy3 take on all this

@davidtheclark
Copy link
Contributor

Over in cosmiconfig I posted a notice that I'm happy to completely hand over the keys to people interested in taking ownership:

MAINTAINERS WANTED! If you're interested in taking over and maintaining Cosmiconfig, please let @davidtheclark know (with an issue or email). I'd like to hand over the keys completely, so I'm looking for owners, not people who just want to merge a PR or two! You can make the decisions about what happens in v8 and subsequent versions, how the package balances stability and opinionated features, and so on. Take a look at open issues and PRs to learn about possibilities that have been on people's minds over the years.

I thought maybe I'd be interested in pushing through v8, but turns out I'm not really, and would rather hand it off at this point.

@ybiquitous
Copy link
Member

@davidtheclark I'm interested in maintaining Cosmiconfig!

@davidtheclark
Copy link
Contributor

@ybiquitous I sent you invites to be a GitHub and npm collaborator.

@ybiquitous
Copy link
Member

@davidtheclark Thanks. I've accepted them. 👍

@jeddy3 jeddy3 changed the title Replace cosmiconfig with lilconfig and js-yaml Refactor to replace cosmiconfig with lilconfig and js-yaml Jul 29, 2022
@d-fischer
Copy link

Node 12 has now been out of support for more than half a year. Also, I see many issues in this repository that hint towards the fact that stylelint will also drop Node 12 support for v15. So, I would really like to drop Node 12 support in cosmiconfig as well, especially since it currently fails cosmiconfig's actions due to npm dropping Node 12 support.

So, I'm asking this, just to be sure: has the stance on Node 12 changed over the last few months?

@ybiquitous
Copy link
Member

@d-fischer Since we've decided to drop Node.js 12 support with the next major version (see #6409), I believe there is no problem that cosmiconfig drops Node.js 12 support.

@d-fischer
Copy link

@ybiquitous cosmiconfig v8 is now out.

@ybiquitous
Copy link
Member

@d-fischer Great, thank you so much!

@ybiquitous ybiquitous linked a pull request Nov 21, 2022 that will close this issue
@ybiquitous
Copy link
Member

PR #6491 updates cosmiconfig to 8.0.0. Note that the PR's base branch is v15. That means Stylelint 15.0.0 will include the new major version of cosmiconfig.

@ybiquitous
Copy link
Member

Closed via #6491

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs discussion triage needs further discussion
Development

Successfully merging a pull request may close this issue.

6 participants