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

Add support for path matcher function #28

Merged
merged 19 commits into from Apr 24, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions index.js
Expand Up @@ -10,9 +10,10 @@ module.exports = (filename, opts = {}) => {

return new Promise(resolve => {
(function find(dir) {
locatePath(filenames, {cwd: dir}).then(file => {
const locating = typeof filename === 'function' ? Promise.resolve(filename(dir)) : locatePath(filenames, {cwd: dir});
locating.then(file => {
if (file) {
resolve(path.join(dir, file));
resolve(path.resolve(dir, file));
} else if (dir === root) {
resolve(null);
} else {
Expand All @@ -31,10 +32,10 @@ module.exports.sync = (filename, opts = {}) => {

// eslint-disable-next-line no-constant-condition
while (true) {
const file = locatePath.sync(filenames, {cwd: dir});
const file = typeof filename === 'function' ? filename(dir) : locatePath.sync(filenames, {cwd: dir});

if (file) {
return path.join(dir, file);
return path.resolve(dir, file);
}

if (dir === root) {
Expand Down
2 changes: 2 additions & 0 deletions package.json
Expand Up @@ -44,6 +44,8 @@
},
"devDependencies": {
"ava": "*",
"is-path-inside": "^2.0.0",
"pify": "^4.0.0",
"tempy": "^0.2.1",
"xo": "*"
}
Expand Down
12 changes: 10 additions & 2 deletions readme.md
Expand Up @@ -47,6 +47,12 @@ const findUp = require('find-up');

console.log(await findUp(['rainbow.png', 'unicorn.png']));
//=> '/Users/sindresorhus/unicorn.png'

console.log(await findUp(dir => Promise.resolve('unicorn.png')));
//=> '/Users/sindresorhus/unicorn.png'

console.log(await findUp(dir => dir));
//=> '/Users/sindresorhus'
sholladay marked this conversation as resolved.
Show resolved Hide resolved
})();
```

Expand All @@ -71,9 +77,11 @@ Returns the first filepath found (by respecting the order) or `null`.

#### filename

Type: `string`
Type: `string` `Function`
sholladay marked this conversation as resolved.
Show resolved Hide resolved

Filename of the file to find, or a custom matcher function to be called with each directory until it returns a filepath to stop the search or the root directory has been reached and nothing was found. When using a matcher function, if you want to check whether a file exists, use [`fs.access()`](https://nodejs.org/api/fs.html#fs_fs_access_path_mode_callback) - this is done automatically when `filename` is a string.
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be useful for us to at least expose convenience methods to check whether the path exists in a normalized way. fs.access() is not obvious to use and you would have to manually promisify it when using it async.

We already depend on path-exists in locate-path, so we could depend on it and expose it here somehow without any increase in dependencies. I think this makes sense as checking whether the path exist is the most common operation you would do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think my documentation made it sound worse than it is. fs.access() isn't really necessary. In most use cases for find-up, it's sufficient to use fs.existsSync() if the filesystem even needs to be checked, and that returns a simple boolean, making it much easier to use here. I've added an example of that to the Usage section instead. Hopefully, a little simpler/clearer now.

I did consider that maybe we should ignore the result of the matcher if the filepath it returns does not exist. But I think it's highly likely that the matcher will be checking the filesystem anyway. And by leaving this up to the matcher, we also enable the user to decide where the search stops in a more deterministic manner (i.e. its decision will never be overridden). I think this is a good foundation to build on top of.

As for convenience methods, are you thinking something like findUp.exists(matcher) / findUp.existsSync(matcher)? Or maybe a pipeline like findUp(matcher, findUp.exists)? My immediate reaction is this merely saves a few characters. If we were to add this, I'd prefer the latter pipeline pattern.


Filename of the file to find.
When using async mode, `filename` may optionally be an `async` function or return a `Promise` for the filepath.

#### options

Expand Down
86 changes: 86 additions & 0 deletions test.js
@@ -1,6 +1,8 @@
import fs from 'fs';
import path from 'path';
import test from 'ava';
import isPathInside from 'is-path-inside';
import pify from 'pify';
import tempy from 'tempy';
import m from '.';

Expand Down Expand Up @@ -244,3 +246,87 @@ test('sync (not found, custom cwd)', t => {

t.is(filePath, null);
});

test('async (custom function)', async t => {
sholladay marked this conversation as resolved.
Show resolved Hide resolved
const cwd = process.cwd();

t.is(await m(dir => {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use the directory wording for all these instead of dir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

t.is(dir, cwd);
return dir;
}), cwd);

t.is(await m(() => {
return '.';
}), cwd);

t.is(await m(() => {
return Promise.resolve('foo.txt');
}), path.join(cwd, 'foo.txt'));
sholladay marked this conversation as resolved.
Show resolved Hide resolved

t.is(await m(() => {
return '..';
}), path.join(cwd, '..'));

t.is(await m(dir => {
return (dir !== cwd) && dir;
}), path.join(cwd, '..'));

t.is(await m(dir => {
return (dir !== cwd) && 'foo.txt';
}), path.join(cwd, '..', 'foo.txt'));

const {root} = path.parse(cwd);
const visited = new Set();
t.is(await m(async dir => {
t.is(typeof dir, 'string');
const stat = await pify(fs.stat)(dir);
t.true(stat.isDirectory());
t.true((dir === cwd) || isPathInside(cwd, dir));
t.false(visited.has(dir));
visited.add(dir);
}), null);
t.true(visited.has(cwd));
t.true(visited.has(root));
sindresorhus marked this conversation as resolved.
Show resolved Hide resolved
});

test('sync (custom function)', t => {
const cwd = process.cwd();

t.is(m.sync(dir => {
t.is(dir, cwd);
return dir;
}), cwd);

t.is(m.sync(() => {
return '.';
}), cwd);

t.is(m.sync(() => {
return 'foo.txt';
}), path.join(cwd, 'foo.txt'));

t.is(m.sync(() => {
return '..';
}), path.join(cwd, '..'));

t.is(m.sync(dir => {
return (dir !== cwd) && dir;
}), path.join(cwd, '..'));

t.is(m.sync(dir => {
return (dir !== cwd) && 'foo.txt';
}), path.join(cwd, '..', 'foo.txt'));

const {root} = path.parse(cwd);
const visited = new Set();
t.is(m.sync(dir => {
t.is(typeof dir, 'string');
const stat = fs.statSync(dir);
t.true(stat.isDirectory());
t.true((dir === cwd) || isPathInside(cwd, dir));
t.false(visited.has(dir));
visited.add(dir);
}), null);
t.true(visited.has(cwd));
t.true(visited.has(root));
});