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

Jump to bottom

Add support for path matcher function #28

Merged
merged 19 commits into from Apr 24, 2019
Merged

Add support for path matcher function #28

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
95d4390
Add support for filename matcher function
sholladay Sep 30, 2018
8b80cf1
Improve documentation
sholladay Dec 28, 2018
e8072bb
Seperate not found tests
sholladay Dec 29, 2018
44e67bf
Bubble up errors correctly
sholladay Dec 29, 2018
102b688
Fix indentation
sholladay Dec 29, 2018
7a83413
Merge branch 'master' of github.com:sindresorhus/find-up
sholladay Jan 7, 2019
6ad0dd8
Fix lint errors
sholladay Jan 7, 2019
ca48c76
Add Symbol for stopping early
sholladay Apr 19, 2019
f858bdd
Merge branch 'master' of github.com:sindresorhus/find-up
sholladay Apr 19, 2019
eff8479
Use util.promisify instead of pify
sholladay Apr 19, 2019
cba6f26
Add documentation and TypeScript types
sholladay Apr 19, 2019
af9d361
Tweak documentation
sindresorhus Apr 19, 2019
b10d5b2
Simplify examples
sholladay Apr 19, 2019
eb97a2f
Merge branch 'master' of github.com:sholladay/find-up
sholladay Apr 19, 2019
ae4d8bc
Use unique symbol
sindresorhus Apr 23, 2019
bb10d62
Ensure symbol is readonly
sholladay Apr 23, 2019
94fafd1
Add test for finding nested dir at path.basename(cwd) (#1)
coreyfarrell Apr 24, 2019
41b8bdb
More strict TypeScript types
sholladay Apr 24, 2019
cf3157c
Update index.d.ts
sindresorhus Apr 24, 2019
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
25 changes: 24 additions & 1 deletion index.d.ts
View file
Open in desktop
Expand Up @@ -9,6 +9,8 @@ declare namespace findUp {
}
}

type Match = string | boolean | null | undefined;
Copy link
Collaborator Author

@sholladay sholladay Apr 19, 2019 •
edited

Choose a reason for hiding this comment

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

I discovered that there's seemingly no way for me to tell TypeScript that the only boolean type that should be allowed is false. Annoying. Should we support returning true? It's easy to support, but I remember we discussed that the semantics may not be obvious.

Also, should I put this type on the namespace?

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 we should type this one as:

type Match = string | typeof findUp.stop | undefined;

While JS is loose, we should try to keep the TS types strict.

Also, should I put this type on the namespace?

Yes

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 agree in general, but in this case, if we are very strict, then people can't necessarily do:

findUp((directory) => {
    return foo && bar;
});

... since foo might be false or null, etc. Is it worth losing that nice syntax?

Copy link
Owner

Choose a reason for hiding this comment

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

TS users will definitely say yes. That's why they're using TS; they want strict types. It doesn't affect the majority that is using plain JS.

TS users can do:

findUp((directory) => {
    return foo ? bar : undefined;
});

It's just a tiny bit more verbose.


declare const findUp: {
/**
Find a file or directory by walking up parent directories.
Expand Down Expand Up @@ -41,13 +43,34 @@ declare const findUp: {
*/
(name: string | string[], options?: findUp.Options): Promise<string | undefined>;

/**
Find a file or directory by walking up parent directories.

@param matcher - Called for each directory in the search. Return a path or `findUp.stop` to stop the search.
@returns The first path found or `undefined` if none could be found.
*/
(matcher: (directory: string) => (Match | Promise<Match>), options?: findUp.Options): Promise<string | undefined>;

/**
Synchronously find a file or directory by walking up parent directories.

@param name - Name of the file or directory to find. Can be multiple.
@returns The first path found (by respecting the order of `names`s) or `undefined` if none could be found.
@returns The first path found (by respecting the order of `name`s) or `undefined` if none could be found.
*/
sync(name: string | string[], options?: findUp.Options): string | undefined;

/**
Synchronously find a file or directory by walking up parent directories.

@param matcher - Called for each directory in the search. Return a path or `findUp.stop` to stop the search.
@returns The first path found or `undefined` if none could be found.
*/
sync(matcher: (directory: string) => Match, options?: findUp.Options): string | undefined;

/**
Return this in a `matcher` function to stop the search and force `findUp` to immediately return `undefined`.
*/
stop: symbol
sholladay marked this conversation as resolved.
Show resolved Hide resolved
};

export = findUp;
21 changes: 16 additions & 5 deletions index.js
View file
Open in desktop
Expand Up @@ -2,18 +2,23 @@
const path = require('path');
const locatePath = require('locate-path');

const stop = Symbol('findUp.stop');

module.exports = async (name, options = {}) => {
let directory = path.resolve(options.cwd || '');
const {root} = path.parse(directory);
const paths = [].concat(name);

// eslint-disable-next-line no-constant-condition
while (true) {
// eslint-disable-next-line no-await-in-loop
const foundPath = await locatePath(paths, {cwd: directory});
const foundPath = await (typeof name === 'function' ? name(directory) : locatePath(paths, {cwd: directory}));

if (foundPath === stop) {
return;
}

if (foundPath) {
return path.join(directory, foundPath);
return path.resolve(directory, foundPath);
}

if (directory === root) {
Expand All @@ -31,10 +36,14 @@ module.exports.sync = (name, options = {}) => {

// eslint-disable-next-line no-constant-condition
while (true) {
const foundPath = locatePath.sync(paths, {cwd: directory});
const foundPath = typeof name === 'function' ? name(directory) : locatePath.sync(paths, {cwd: directory});

if (foundPath === stop) {
return;
}

if (foundPath) {
return path.join(directory, foundPath);
return path.resolve(directory, foundPath);
}

if (directory === root) {
Expand All @@ -44,3 +53,5 @@ module.exports.sync = (name, options = {}) => {
directory = path.dirname(directory);
}
};

module.exports.stop = stop;
sholladay marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 26 additions & 0 deletions index.test-d.ts
View file
Open in desktop
Expand Up @@ -5,8 +5,34 @@ expectType<Promise<string | undefined>>(findUp('unicorn.png'));
expectType<Promise<string | undefined>>(findUp('unicorn.png', {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(['rainbow.png', 'unicorn.png']));
expectType<Promise<string | undefined>>(findUp(['rainbow.png', 'unicorn.png'], {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(() => 'unicorn.png'));
expectType<Promise<string | undefined>>(findUp(() => 'unicorn.png', {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(() => false));
expectType<Promise<string | undefined>>(findUp(() => false, {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(() => null));
expectType<Promise<string | undefined>>(findUp(() => null, {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(() => undefined));
expectType<Promise<string | undefined>>(findUp(() => undefined, {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(async () => 'unicorn.png'));
expectType<Promise<string | undefined>>(findUp(async () => 'unicorn.png', {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(async () => false));
expectType<Promise<string | undefined>>(findUp(async () => false, {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(async () => null));
expectType<Promise<string | undefined>>(findUp(async () => null, {cwd: ''}));
expectType<Promise<string | undefined>>(findUp(async () => undefined));
expectType<Promise<string | undefined>>(findUp(async () => undefined, {cwd: ''}));

expectType<string | undefined>(findUp.sync('unicorn.png'));
expectType<string | undefined>(findUp.sync('unicorn.png', {cwd: ''}));
expectType<string | undefined>(findUp.sync(['rainbow.png', 'unicorn.png']));
expectType<string | undefined>(findUp.sync(['rainbow.png', 'unicorn.png'], {cwd: ''}));
expectType<string | undefined>(findUp.sync(() => 'unicorn.png'));
expectType<string | undefined>(findUp.sync(() => 'unicorn.png', {cwd: ''}));
expectType<string | undefined>(findUp.sync(() => false));
expectType<string | undefined>(findUp.sync(() => false, {cwd: ''}));
expectType<string | undefined>(findUp.sync(() => null));
expectType<string | undefined>(findUp.sync(() => null, {cwd: ''}));
expectType<string | undefined>(findUp.sync(() => undefined));
expectType<string | undefined>(findUp.sync(() => undefined, {cwd: ''}));

expectType<Symbol>(findUp.stop);
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@
},
"devDependencies": {
"ava": "^1.4.1",
"is-path-inside": "^2.1.0",
"tempy": "^0.3.0",
"tsd": "^0.7.2",
"xo": "^0.24.0"
Expand Down
40 changes: 40 additions & 0 deletions readme.md
View file
Open in desktop
Expand Up @@ -39,6 +39,8 @@ $ npm install find-up
`example.js`

```js
const fs = require('fs');
const path = require('path');
const findUp = require('find-up');

(async () => {
Expand All @@ -47,13 +49,28 @@ const findUp = require('find-up');

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

console.log(await findUp(dir => {
return fs.existsSync(path.join(dir, 'unicorn.png')) && 'foo';
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't mix async and sync. Here, it should have used an async method to check whether it exists. The problem is that fs.exists is deprecated, and it's also not async/await friendly. This is why I want to expose some utility methods. To make common operation simple.

I'm thinking:

  • findUp.exists()
  • findUp.isFile()
  • findUp.isDirectory()
  • findUp.sync.exists()
  • findUp.sync.isFile()
  • findUp.sync.isDirectory()

I know this seems like a lot, but I want find-up to just work for people with minimal mental overhead or boilerplate.

Maybe we don't need the exists methods as I can't really think of a scenario where you wouldn't care about the type. Can you?

The methods could alternatively be passed into the callback as a sort of context or something. I think I prefer them to be on the findUp object though.

Copy link
Owner

Choose a reason for hiding this comment

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

Or maybe we should have a type option that accepts either file or directory, but defaults to file? Hmm, we should probably have that regardless, as currently find-up could match both directories and files, while most would only want to match file. Usually it's not a problem as people use extensions, but I can imagine a scenario where a user wants to find an extension-less file and instead gets a directory of the same name somewhere else in the hierarchy.

Copy link
Collaborator Author

@sholladay sholladay Apr 18, 2019 •
edited

Choose a reason for hiding this comment

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

Yeah, I should've used findUp.sync(). I think I was trying to avoid that and then I realized:

  1. As you mentioned, async fs.exists() is deprecated.
  2. At the time, converting from callbacks to promises would have been some extra noisy code to understand for any async fs method.

But now fs.promises is stable and it makes the examples simpler. How about something like this?

const pathExists = filepath => fs.promises.access(filepath).then(_ => true).catch(_ => false);
console.log(await findUp(async (directory) => {
	const isInstalled = await pathExists(path.join(directory, 'node_modules'));
	return isInstalled && 'package.json';
}});

That could replace the example below as well.

As for the utility methods, I agree they are useful and I'd also prefer for them to be properties on the findUp function. Let's do that. I think we should merge this PR first and then build the utilities on top of this functionality in a separate PR(s), since this is still useful with or without them.

Copy link
Owner

Choose a reason for hiding this comment

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

How about something like this?

👍

I think we should merge this PR first and then build the utilities on top of this functionality in a separate PR(s), since this is still useful with or without them.

👍

Copy link
Owner

Choose a reason for hiding this comment

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

I realized that if we implement #33, we don't need the file/directory checks, so we can simply implement two utility methods => #37

}));
//=> '/Users/sindresorhus/foo'

console.log(await findUp(async dir => {
const children = await fs.promises.readdir(dir);
if (children.some(fileName => fileName.endsWith('.png'))) {
return dir;
}
}));
//=> '/Users/sindresorhus'
sholladay marked this conversation as resolved.
Show resolved Hide resolved
})();
```


## API


### findUp(name, [options])
### findUp(matcher, [options])

Returns a `Promise` for either the path or `undefined` if it couldn't be found.

Expand All @@ -62,6 +79,7 @@ Returns a `Promise` for either the path or `undefined` if it couldn't be found.
Returns a `Promise` for either the first path found (by respecting the order) or `undefined` if none could be found.

### findUp.sync(name, [options])
### findUp.sync(matcher, [options])

Returns a path or `undefined` if it couldn't be found.

Expand All @@ -75,6 +93,14 @@ Type: `string`

Name of the file or directory to find.

#### matcher

Type: `Function`

A function that will 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. Useful if you want to match files with a certain pattern, set of permissions, or other advanced use cases.
sholladay marked this conversation as resolved.
Show resolved Hide resolved

When using async mode, `matcher` may optionally be an `async` function or return a `Promise` for the filepath.
sholladay marked this conversation as resolved.
Show resolved Hide resolved

#### options

Type: `object`
Expand All @@ -86,6 +112,20 @@ Default: `process.cwd()`

Directory to start from.

### findUp.stop

A [Symbol](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol) that can be returned by a `matcher` function to stop the search and cause `findUp` to immediately return `undefined`. Useful as a performance optimization in case the current working directory is deeply nested in the filesystem.

```js
const path = require('path');
const findUp = require('find-up');

(async () => {
await findUp((directory) => {
return path.basename(directory) === 'work' ? findUp.stop : 'logo.png';
});
})();
```

## Security

Expand Down