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: replace cosmiconfig with lilconfig #183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Jan 7, 2022

What:

cosmiconfig is a massive package which recently got a small alternative
without dependencies lilconfig.

https://github.com/antonk52/lilconfig

https://packagephobia.com/result?p=cosmiconfig
https://packagephobia.com/result?p=lilconfig

Though still a big part of cosmiconfig is yaml package.
https://packagephobia.com/result?p=yaml

This package is important piece of relay and emotion. Would be good to make it as small as possible.

Why:

Node modules are bloated with large dependencies. I'm trying to help with this.

How:

Just switch to alternative.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

@TrySound
Copy link
Contributor Author

TrySound commented Jan 7, 2022

cc @kentcdodds

@kentcdodds
Copy link
Owner

Thanks for this! Is this a breaking change?

@TrySound
Copy link
Contributor Author

TrySound commented Jan 7, 2022

Shouldn't be. lilconfig is mostly compatible. Yaml loaders are covered.
Not sure why tests are failing. Looks like something unrelated to change.

@kentcdodds
Copy link
Owner

Bummer, looks like this project is using old versions of deps and something broke. I'm afraid I don't have any time to get things updated in this project and get things fixed so we can get this merged :(

@TrySound
Copy link
Contributor Author

TrySound commented Jan 7, 2022

If you want I can take care of this on weekend.

@conartist6
Copy link
Collaborator

It looks nonbreaking since it still requires yaml and defines a yaml loader. I really wouldn't mind seeing yaml config support dropped.

Would it work to just have yaml as an optional peer dependency?

@kentcdodds
Copy link
Owner

Whatever you two want to do. I don't use babel anymore.

@TrySound
Copy link
Contributor Author

TrySound commented Jan 7, 2022

I don't personally use macros support but care about node_modules size so it's enough motivation. Will look tomorrow.

@conartist6
Copy link
Collaborator

From what you shared just changing to lilconfig and yaml would save ~275kb in node_modules (751 -> 466KB of deps for config loading). It's a moderate win, but not nearly as big as also eliminating the useless yaml dep, at which point we'd be down to 27kb of config loading which finally sounds like a number on the correct order of magnitude.

@TrySound
Copy link
Contributor Author

TrySound commented Jan 9, 2022

I fixed build here. Though not sure as ci run need to be approved.
#184

@conartist6
Copy link
Collaborator

conartist6 commented Jan 10, 2022

You'll also need to change

jest.mock('cosmiconfig', () => {
const cosmiconfigExports = jest.requireActual('cosmiconfig')
const actualCosmiconfigSync = cosmiconfigExports.cosmiconfigSync
function fakeCosmiconfigSync(...args) {
fakeCosmiconfigSync.explorer = actualCosmiconfigSync(...args)
return fakeCosmiconfigSync.explorer
}
return {...cosmiconfigExports, cosmiconfigSync: fakeCosmiconfigSync}
})

What the heck is that for I wonder!?

"resolve": "^1.19.0"
"lilconfig": "^2.0.4",
"resolve": "^1.19.0",
"yaml": "^1.10.2"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is just a recent security vulnerability on yaml <2.2.2, is it possible to take an upgraded version? Thanks
https://security.snyk.io/vuln/SNYK-JS-YAML-5458867

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

Successfully merging this pull request may close these issues.

None yet

4 participants