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
Comments
@binyamin Thanks for the proposal. It sounds good to me. 👍🏼 Do you know details about compatibility between Our documentation has mentioned
|
@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.
(ref) |
@binyamin Thank you. If not 100% compatible, it seems reasonable to me that we would include this change in the next major version ( Any thoughts? |
@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. |
|
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 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
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 |
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:
I wonder if it's worth exploring using |
@ntwb Thanks for sharing the historical context.
I'm not sure the replacement reason, but there were some security issues for |
Good catch @ybiquitous, it appears to be from security issues: Via cosmiconfig/cosmiconfig#216 (comment)
Seems the |
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. |
@binyamin @ntwb Thanks for clarifying the discussion. It sounds better to me to continue the maintenance of I wish @davidtheclark would give someone (who has a passion for the maintenance of This is just one of my opinions, though. |
👋 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! |
@davidtheclark Good news! 🙌🏼 What do you think about updating the |
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. |
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?
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? |
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
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 What is your opinion on these above? |
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
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.
If this will have (for most users) limited impact, then per this:
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 |
Over in cosmiconfig I posted a notice that I'm happy to completely hand over the keys to people interested in taking ownership:
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. |
@davidtheclark I'm interested in maintaining Cosmiconfig! |
@ybiquitous I sent you invites to be a GitHub and npm collaborator. |
@davidtheclark Thanks. I've accepted them. 👍 |
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? |
@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. |
@ybiquitous cosmiconfig v8 is now out. |
@d-fischer Great, thank you so much! |
PR #6491 updates |
Closed via #6491 |
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 calledjs-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 thecosmiconfig
API. The main exception is yaml parsing, which can be easily added.Solution
Reduce bundle size dramatically, by replacing
cosmiconfig
with a combination oflilconfig
andjs-yaml
.The text was updated successfully, but these errors were encountered: