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

Adding caching support #42

Closed
pmdartus opened this issue Nov 14, 2023 · 7 comments · Fixed by #43
Closed

Adding caching support #42

pmdartus opened this issue Nov 14, 2023 · 7 comments · Fixed by #43

Comments

@pmdartus
Copy link

As mentioned in the README, lilconfig doesn't currently support caching. It would be possible to greatly improve Prettier CLI performance if lilconfig were to implement some caching capabilities. Based on the analysis I have done, adding caching support could potentially improve Prettier CLI performance by ~15% on a large-scale project. More details can be found here: prettier/prettier#15653.

@antonk52 Are you open to adding caching capabilities to lilconfig in a similar fashion as cosmiconfig? I wanted to check with you first before I spend time working on this.

@antonk52
Copy link
Owner

antonk52 commented Nov 14, 2023

Hi, @pmdartus and thank you for the issue.

First of all, I was not aware that prettier started using lilconfig. I am pleasantly surprised.

To clarify what you linked from the readme the configs loaded by lilconfig are cached within the same process. What was mentioned in the readme is actually "module cache invalidation". Cosmiconfig uses a package import-fresh which clears nodejs' require cache and re-requires the config file which also causes re execution of the config file. If the content of the config file do not change this should be more performant than re-requiring and re executing it every time. So it is not caching of config location, but require's cache if you call cosmiconfig's search/find multiple times in a single process.

I have looked at the prettier issue you linked. If I understand it correctly you would like to cache the config location between prettier invocations. Please correct me if I misunderstood it. It is not immediately clear to me how this can be implemented in lilconfig/cosmiconfig without writing to a file system which is quite a side effect that I would prefer to avoid if possible.

Does this clarify your concern about what type of caching is missing?

PS: I am going to update the readme to avoid this confusion in the future.

UPDATE: I have hurried too much to answer this. I have looked at the cosmiconfig's codebase and can see that they do have caching of config location within the same process. I think this is a fair ask to add this feature to lilconfig. Would you like to have a go at it? Otherwise I can git it a try over the weekend.

@ondrej-langr
Copy link

ondrej-langr commented Nov 14, 2023

Hey! I want to add one more thing to this (on the note prettier + lilconfig instead of cosmiconfig) - in our internal project we kind of relied on cosmiconfig to keep config cached for files on the same process even though original file (that has been config cached for) does not exist anymore (we delete original file and create a new one with fresh contents)

As you may guess we do this for prettier config too which screws up entire process and CLI application fails altogether (we run multiple file recreations synchronously to save time on entire process)

Im not so sure about adding this caching into lilconfig at all, but instead adding caching to prettier itself before lilconfig (or atleast cache config based on file type/filename instead of entire path)

@fisker
Copy link

fisker commented Nov 14, 2023

I haven't check how cosmiconfig implement the cache.

But this is what in my mind, eg:

/a/b/file's config file is /a/config, then any file between /a and /a/b should use the same config file.

when search /a/b/c/file, since it's not between /a and /a/b/, skip cache.

@ondrej-langr
Copy link

That would be adequate - but the question is if this should be supported here in lilconfig or ouside of this (prettier) Not sure on what direction this library wants to take this

@antonk52
Copy link
Owner

That would be adequate - but the question is if this should be supported here in lilconfig or ouside of this (prettier) Not sure on what direction this library wants to take this

As noted above, I (lilconfig's founder/maintainer) am happy for lilconfig to add caching capabilities. It definitely makes sense to me as it would also speed things up. The cache strategy should generally align with cosmiconfig's one.

Regardless of lilconfig's implementation prettier can still implement it's own caching layer as every lilconfig result provides a config and its path, prettier can implement any caching heuristics that would have the most sense for its use case.

@antonk52 antonk52 mentioned this issue Nov 15, 2023
7 tasks
@pmdartus
Copy link
Author

Regardless of lilconfig's implementation prettier can still implement it's own caching layer as every lilconfig result provides a config and its path, prettier can implement any caching heuristics that would have the most sense for its use case.

Yes, it's the plan. Some caching will still be required to be implemented on top of this, at Prettier CLI level, to speed up the option resolution.

Thanks again for jumping on this @antonk52. Greatly appreciated! 😉

@fisker
Copy link

fisker commented Nov 15, 2023

Some caching will still be required to be implemented on top of this, at Prettier CLI level, to speed up the option.

Not true, it can be done on Prettier side, but nice to have it out of box.

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 a pull request may close this issue.

4 participants