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

Upgrade cosmiconfig to 3.x #282

Closed
okonet opened this issue Sep 15, 2017 · 18 comments
Closed

Upgrade cosmiconfig to 3.x #282

okonet opened this issue Sep 15, 2017 · 18 comments

Comments

@okonet
Copy link
Collaborator

okonet commented Sep 15, 2017

https://github.com/davidtheclark/cosmiconfig 2.x offers caching and transform function that should potentially make #262 obsolete.

@okonet
Copy link
Collaborator Author

okonet commented Sep 15, 2017

https://github.com/okonet/lint-staged/blob/master/package.json#L74 can be removed as well if we do that.

@sudo-suhas
Copy link
Collaborator

3.0 is going to be released soon. We should update to that.

@okonet okonet changed the title Upgrade cosmiconfig to 2.x Upgrade cosmiconfig to 3.x Sep 15, 2017
@okonet
Copy link
Collaborator Author

okonet commented Sep 15, 2017

Agreed! Let's wait.

@okonet
Copy link
Collaborator Author

okonet commented Sep 15, 2017

@sudo-suhas I didn't realize you're #2 contributor of cosmiconfig! 🥇

@okonet
Copy link
Collaborator Author

okonet commented Sep 15, 2017

For that reason I've assigned you ;) Hope you don't mind.

@sudo-suhas
Copy link
Collaborator

Ha ha.. It would be a pleasure 😁

@luftywiranda13
Copy link
Collaborator

yesss, we have @sudo-suhas here 🎉

@sudo-suhas
Copy link
Collaborator

@okonet cosmiconfig 3.0 has been released. I want to understand a couple of things before upgrading.

Why do you think we should use caching and transform?
From the cosmiconfig readme

The reason you might use this option instead of simply applying your transform function some other way is that the transformed result will be cached.

I think the cache and transform would have been relevant if we were invoking explorer.load multiple times.

cosmiconfig now also supports loading the config synchronously. Would you like me to use that?

@okonet
Copy link
Collaborator Author

okonet commented Sep 17, 2017

I was under impression that using cache and transform will cache the transformed full config once (on the file system) and won't do that for any consequent calls. Since lint-staged config is rarely changed, I think it could improve the overall time of execution but I might be wrong. Also, transform will save us some work on getConfig optimization I hoped.

@okonet
Copy link
Collaborator Author

okonet commented Sep 17, 2017

I don't think we should use sync mode besides there are clear benefits of doing this. I'd like to stick the code style to be promise based for most parts.

@sudo-suhas
Copy link
Collaborator

The cache is in-memory, not useful for our usage right now. But the file-system cache is a good idea. I might try to implement that in cosmiconfig.

@okonet
Copy link
Collaborator Author

okonet commented Sep 17, 2017

Good to know. Not sure how much benefit it will gain without inspecting the bottlenecks first.

@okonet
Copy link
Collaborator Author

okonet commented Sep 17, 2017

Wondering what was the use-case for caching in the first place.

@sudo-suhas
Copy link
Collaborator

If you have something like stylelint, it has to call explorer.load repeatedly for different files or directories. So caching what you know about the config files you found should help. I only started working on cosmiconfig recently so I am just guessing. @davidtheclark will know better.

@davidtheclark
Copy link

If you have something like stylelint, it has to call explorer.load repeatedly for different files or directories.

Yep. The reason I introduced caching was because stylelint wanted to switch from looking up a single configuration, once, to looking up a configuration for every particular file — allowing for different directories to use different configurations, but all linted in the same process. stylelint calls explorer.load, then, for every single file it analyzes. That's when in-memory caching should pay off.

@okonet
Copy link
Collaborator Author

okonet commented Sep 18, 2017

Okay so it seems to be a not so good fit for us...

@jksmithing
Copy link

Want to note, too, that cosmicconfig@3 removes support for --config, which is useful when running lint-staged from a directory that is not process.cwd() (i.e. when used as part of another package). Maybe we could add support for --config to lint-staged if we do upgrade to cosmicconfig@3.

@cameronhunter
Copy link
Contributor

@neonsilk PR #304 adds support for --config

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

No branches or pull requests

6 participants