Cache option for searchers

[antonk52]

This PR introduces a new option cache(it should have been there from the start). The API and implementation is inspired by cosmiconfig. Similarities between lilconfig and cosmiconfig implementation:

• Both use maps for storing caching results
• The same public API to clear searchCache/loadCache/both
• cache option is enabled by default
• sync and async load methods use filepath argument as cache key (cosmiconfig)
• sync and async search methods use path to a directory where lookup for search places will take place as cache key (cosmiconfig from is reassigned and called recursively in a closure)

I have added test cases for cache option enabled and disabled. Jest reports test coverage to be 100%. I am happy to add more tests if anyone can spot any missed cases.

Once approved I will merge this PR and release this as a new major version since this option is enabled by default. I consider this change in behaviour a breaking change. A few more changes will be merged prior to releasing a new version like dropping support for legacy node.js versions.

TODO

• async implementation
• sync implementation
• test enabled option
• test disabled option
• add to readme
• compare to cosmiconfig implementation
• add searchers shape comparison in tests

fixes #42

[pmdartus]

I know this PR is still in draft. But I thought that you might appreciate some early feedback.
Very excited about this feature landing. 👏

[pmdartus]

If lilconfig is configured with multiple searchPlaces, this code will lookup for the same cache entry for each search place. In the case of Prettier, 14 different searchPlaces are passed.

Since Map.prototype.has is very efficient, it shouldn't make a big difference performance-wise. But it be ideal to avoid this.

[antonk52]

I am using searchPath as a cache key, not filepath. On each iteration searchPath is the directory where we expect the config to be, not the full path to the config file. This keeps the code simpler and we will have less entries in the cache as a result. Example values from a loop iteration:

searchPath -> /foo/bar/baz
filepath -> /foo/bar/baz/prettier.config.js

I agree that the naming is rather confusing. I haven't came up with a better names.

[pmdartus]

Thanks for the clarification. I think my comment is still valid, here is a concrete example:

const resolver = lilconfig('config', {
  searchPlaces: ['package.json', '.prettierrc', '.prettierrc.json'],
});

resolver.search('./foo');

In the above example, getSearchItems would generate the following entries:

• { searchPath: './foo', filepath: './foo/package.json' }
• { searchPath: './foo', filepath: './foo/.prettierrc' }
• { searchPath: './foo', filepath: './foo/.prettierrc.json' }
• { searchPath: './', filepath: './package.json' }
• { searchPath: './', filepath: './.prettierrc' }
• { searchPath: './', filepath: './.prettierrc.json' }

Correct me if I am wrong but with the for loop iterating over the entries generated by getSearchItems, when cache is enabled, it appears to me that this code will do:

• 3 consecutive cache lookup with: searchCache.get('./foo')
• follow by, 3 consecutive cache lookup with: searchCache.get('./')

IMO, it's not a big deal performance-wise since the has check on a Map is not very expensive. But ideally, the code should do 1 lookup for each searchpath in the search cache.

[antonk52]

Thanks for expanding on this. I've switched the look up for two nested loops. No more unnecessary look ups

[pmdartus]

Thanks for the clarification. I think my comment is still valid, here is a concrete example:

const resolver = lilconfig('config', {
  searchPlaces: ['package.json', '.prettierrc', '.prettierrc.json'],
});

resolver.search('./foo');

In the above example, getSearchItems would generate the following entries:

• { searchPath: './foo', filepath: './foo/package.json' }
• { searchPath: './foo', filepath: './foo/.prettierrc' }
• { searchPath: './foo', filepath: './foo/.prettierrc.json' }
• { searchPath: './', filepath: './package.json' }
• { searchPath: './', filepath: './.prettierrc' }
• { searchPath: './', filepath: './.prettierrc.json' }

Correct me if I am wrong but with the for loop iterating over the entries generated by getSearchItems, when cache is enabled, it appears to me that this code will do:

• 3 consecutive cache lookup with: searchCache.get('./foo')
• follow by, 3 consecutive cache lookup with: searchCache.get('./')

IMO, it's not a big deal performance-wise since the has check on a Map is not very expensive. But ideally, the code should do 1 lookup for each searchpath in the search cache.

[pmdartus]

A few more changes will be merged prior to releasing a new version like dropping support for legacy node.js versions.

As a side note, Prettier still supports Node 14 today.

I don't know what is the plan for Prettier to dropping Node 14 and Node 16 (both EOL). I know it's not ideal, but it would be great if lilconfig could still maintain backward compat with Node 14.

[antonk52]

I think this PR is ready to be merged. I ran a few benchmarks in feat/cache-bench branch. And made a couple of more small touches around getting from and updating the cache. Overall I am happy about the implementation and tests.

There is a small dip in test coverage because of an edge case that only occurs in nixos. I am not sure how this can be tested, so ignoring it for now.

While I am confident about basic benchmarks with set and map, the benchmarks with searches seem ludicrous to me. I might have messed up something with benchmark.js. I have no doubt about the new implementation being faster than the old one, but these numbers show too much of a difference to be believable.

Running "sync single search"
old x 959 ops/sec ±1.01% (93 runs sampled)
new x 31,080,130 ops/sec ±0.36% (97 runs sampled)
Fastest is new