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.

Sign up for GitHub

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

Cache option for searchers #43

Merged
merged 17 commits into from
Nov 18, 2023
Merged

Cache option for searchers #43

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0c124d7
run prettier in test file
antonk52 Nov 14, 2023
0ebf699
cache implementation
antonk52 Nov 15, 2023
148b677
test enabled cache option
antonk52 Nov 15, 2023
5ba50ab
make sure cache is always populated
antonk52 Nov 15, 2023
2090b6c
update docs for cache options
antonk52 Nov 15, 2023
5c4a141
compare exporers shape completely
antonk52 Nov 15, 2023
b073423
restructure tests + cache off tests
antonk52 Nov 15, 2023
d9721f4
remove unused type annotation
antonk52 Nov 15, 2023
de7ad2a
refactor to two loops for more efficient look up
antonk52 Nov 16, 2023
5c6e3ac
fix double cache check
antonk52 Nov 16, 2023
abe9528
tests for no extension loader
antonk52 Nov 16, 2023
f2a4440
remove cache check in clear cache functions
antonk52 Nov 16, 2023
92a8c61
Revert "remove cache check in clear cache functions"
antonk52 Nov 16, 2023
903e561
do not check for existence in visited set before adding
antonk52 Nov 17, 2023
f2b1a1a
checking for undefined is faster than looking up key in a map again
antonk52 Nov 17, 2023
cad8f7a
use parentDir util instead of path.dirname
antonk52 Nov 17, 2023
d0fbdfe
lower branch test coverage threshhold to 98.9%
antonk52 Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 153 additions & 30 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export type LilconfigResult = null | {
};

interface OptionsBase {
cache?: boolean;
stopDir?: string;
searchPlaces?: string[];
ignoreEmptySearchPlaces?: boolean;
Expand Down Expand Up @@ -101,6 +102,7 @@ function getOptions(
stopDir: os.homedir(),
searchPlaces: getDefaultSearchPlaces(name),
ignoreEmptySearchPlaces: true,
cache: true,
antonk52 marked this conversation as resolved.
Show resolved Hide resolved
transform: (x: LilconfigResult): LilconfigResult => x,
packageProp: [name],
...options,
Expand Down Expand Up @@ -147,6 +149,7 @@ type SearchItem = {
filepath: string;
searchPlace: string;
loaderKey: string;
searchPath: string;
};

function getSearchItems(
Expand All @@ -156,6 +159,7 @@ function getSearchItems(
return searchPaths.reduce<SearchItem[]>((acc, searchPath) => {
searchPlaces.forEach(sp =>
acc.push({
searchPath,
searchPlace: sp,
filepath: path.join(searchPath, sp),
loaderKey: path.extname(sp) || 'noExt',
Expand All @@ -176,10 +180,25 @@ function validateLoader(loader: Loader, ext: string): void | never {
throw new Error('loader is not a function');
}

type ClearCaches = {
clearLoadCache: () => void;
clearSearchCache: () => void;
clearCaches: () => void;
};

const makeEmplace =
<T extends LilconfigResult | Promise<LilconfigResult>>(
enableCache: boolean,
) =>
(c: Map<string, T>, filepath: string, res: T): T => {
if (enableCache) c.set(filepath, res);
return res;
};

type AsyncSearcher = {
search(searchFrom?: string): Promise<LilconfigResult>;
load(filepath: string): Promise<LilconfigResult>;
};
} & ClearCaches;

export function lilconfig(
name: string,
Expand All @@ -192,7 +211,12 @@ export function lilconfig(
searchPlaces,
stopDir,
transform,
cache,
} = getOptions(name, options);
type R = LilconfigResult | Promise<LilconfigResult>;
const searchCache = new Map<string, R>();
const loadCache = new Map<string, R>();
const emplace = makeEmplace<R>(cache);

return {
async search(searchFrom = process.cwd()): Promise<LilconfigResult> {
Expand All @@ -204,7 +228,22 @@ export function lilconfig(
};

const searchItems = getSearchItems(searchPlaces, searchPaths);
for (const {searchPlace, filepath, loaderKey} of searchItems) {
const visited: Set<string> = new Set();
for (const {
searchPlace,
filepath,
loaderKey,
searchPath,
} of searchItems) {
if (cache) {
if (searchCache.has(searchPath)) {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

return searchCache.get(searchPath) as
antonk52 marked this conversation as resolved.
Show resolved Hide resolved
| LilconfigResult
| Promise<LilconfigResult>;
} else {
visited.has(searchPath) || visited.add(searchPath);
}
}
try {
await fs.promises.access(filepath);
} catch {
Expand Down Expand Up @@ -241,15 +280,26 @@ export function lilconfig(
break;
}

// not found
if (result.filepath === '' && result.config === null)
return transform(null);
const transformed: LilconfigResult | Promise<LilconfigResult> =
// not found
result.filepath === '' && result.config === null
? transform(null)
: transform(result);

if (cache) {
for (const p of visited) {
emplace(searchCache, p, transformed);
}
}

return transform(result);
return transformed;
},
async load(filepath: string): Promise<LilconfigResult> {
validateFilePath(filepath);
const absPath = path.resolve(process.cwd(), filepath);
if (cache && loadCache.has(absPath)) {
return loadCache.get(absPath) as LilconfigResult;
}
const {base, ext} = path.parse(absPath);
const loaderKey = ext || 'noExt';
const loader = loaders[loaderKey];
Expand All @@ -258,10 +308,14 @@ export function lilconfig(

if (base === 'package.json') {
const pkg = await loader(absPath, content);
return transform({
config: getPackageProp(packageProp, pkg),
filepath: absPath,
});
return emplace(
loadCache,
absPath,
transform({
config: getPackageProp(packageProp, pkg),
filepath: absPath,
}),
);
}
const result: LilconfigResult = {
config: null,
Expand All @@ -270,28 +324,48 @@ export function lilconfig(
// handle other type of configs
const isEmpty = content.trim() === '';
if (isEmpty && ignoreEmptySearchPlaces)
return transform({
config: undefined,
filepath: absPath,
isEmpty: true,
});
return emplace(
loadCache,
absPath,
transform({
config: undefined,
filepath: absPath,
isEmpty: true,
}),
);

// cosmiconfig returns undefined for empty files
result.config = isEmpty
? undefined
: await loader(absPath, content);

return transform(
isEmpty ? {...result, isEmpty, config: undefined} : result,
return emplace(
loadCache,
absPath,
transform(
isEmpty ? {...result, isEmpty, config: undefined} : result,
),
);
},
clearLoadCache() {
if (cache) loadCache.clear();
antonk52 marked this conversation as resolved.
Show resolved Hide resolved
},
clearSearchCache() {
if (cache) searchCache.clear();
},
clearCaches() {
if (cache) {
loadCache.clear();
searchCache.clear();
}
},
};
}

type SyncSearcher = {
search(searchFrom?: string): LilconfigResult;
load(filepath: string): LilconfigResult;
};
} & ClearCaches;

export function lilconfigSync(
name: string,
Expand All @@ -304,7 +378,12 @@ export function lilconfigSync(
searchPlaces,
stopDir,
transform,
cache,
} = getOptions(name, options);
type R = LilconfigResult;
const searchCache = new Map<string, R>();
const loadCache = new Map<string, R>();
const emplace = makeEmplace<R>(cache);

return {
search(searchFrom = process.cwd()): LilconfigResult {
Expand All @@ -315,8 +394,21 @@ export function lilconfigSync(
filepath: '',
};

const visited: Set<string> = new Set();
const searchItems = getSearchItems(searchPlaces, searchPaths);
for (const {searchPlace, filepath, loaderKey} of searchItems) {
for (const {
searchPlace,
filepath,
loaderKey,
searchPath,
} of searchItems) {
if (cache) {
if (searchCache.has(searchPath)) {
return searchCache.get(searchPath) as LilconfigResult;
} else {
visited.has(searchPath) || visited.add(searchPath);
}
}
try {
fs.accessSync(filepath);
} catch {
Expand Down Expand Up @@ -353,15 +445,26 @@ export function lilconfigSync(
break;
}

// not found
if (result.filepath === '' && result.config === null)
return transform(null);
const transformed =
// not found
result.filepath === '' && result.config === null
? transform(null)
: transform(result);

if (cache) {
for (const p of visited) {
emplace(searchCache, p, transformed);
}
}

return transform(result);
return transformed;
},
load(filepath: string): LilconfigResult {
validateFilePath(filepath);
const absPath = path.resolve(process.cwd(), filepath);
if (cache && loadCache.has(absPath)) {
return loadCache.get(absPath) as LilconfigResult;
}
const {base, ext} = path.parse(absPath);
const loaderKey = ext || 'noExt';
const loader = loaders[loaderKey];
Expand All @@ -383,18 +486,38 @@ export function lilconfigSync(
// handle other type of configs
const isEmpty = content.trim() === '';
if (isEmpty && ignoreEmptySearchPlaces)
return transform({
filepath: absPath,
config: undefined,
isEmpty: true,
});
return emplace(
loadCache,
absPath,
transform({
filepath: absPath,
config: undefined,
isEmpty: true,
}),
);

// cosmiconfig returns undefined for empty files
result.config = isEmpty ? undefined : loader(absPath, content);

return transform(
isEmpty ? {...result, isEmpty, config: undefined} : result,
return emplace(
loadCache,
absPath,
transform(
isEmpty ? {...result, isEmpty, config: undefined} : result,
),
);
},
clearLoadCache() {
if (cache) loadCache.clear();
},
clearSearchCache() {
if (cache) searchCache.clear();
},
clearCaches() {
if (cache) {
loadCache.clear();
searchCache.clear();
}
},
};
}
Loading
Loading