Add support for path matcher function #28

sholladay · 2018-09-30T20:16:41Z

Closes #21
Closes #36

This PR adds a new call signature with a matcher function, where matcher receives the absolute path of the current directory being searched and it returns a path to finish the search. The matched path will be resolved against the directory and returned to the caller of findUp(). If matcher returns a falsey value, the search continues up to the next directory with the same semantics as findUp has always had when there are no matches.

Miscellaneous notes:

Promises are supported! That should make filesystem calls much more ergonomic.
I added a findUp.stop symbol (see Add support for path matcher function #28 (comment)) so users can return early from the search without matching a particular path (i.e. findUp will return undefined when matcher returns the stop symbol).
The algorithm does not do an existence check on the matched path. At first glance, this might seem a bit inconsistent, but I believe the flexibility is worth it and I propose that an existence check is an implementation detail of the matcher function.

sholladay · 2018-10-07T01:16:19Z

Friendly ping @sindresorhus, just in case you didn't see this. No rush, but should be a pretty quick review. Mostly just documentation and tests.

readme.md

test.js

readme.md

sindresorhus · 2018-12-27T17:21:29Z

readme.md

-Type: `string`
+Type: `string` `Function`
+
+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.


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.

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.

I believe all issues have been addressed

sholladay · 2018-12-29T03:26:08Z

Ready for review again @sindresorhus. At this point my main question is: are convenience methods a blocker for this PR? If so, what sort of pattern do you want to use?

sholladay · 2019-01-07T01:24:25Z

Fixed the lint errors.

So awesome to see the full test matrix: multiple Node versions, all major operating systems, running in parallel, with simple config, all on one unified platform. :D

sindresorhus · 2019-04-18T10:32:42Z

readme.md

@@ -47,13 +49,27 @@ 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';


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.

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.

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

As you mentioned, async fs.exists() is deprecated.

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.

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.

👍

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

readme.md

sindresorhus · 2019-04-18T11:06:16Z

test.js

+test('async (matcher function)', async t => {
+	const cwd = process.cwd();
+
+	t.is(await m(dir => {


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

sindresorhus · 2019-04-18T11:08:43Z

I did not implement a Symbol for stopping the search. I don't think it's necessary. Users can just match the current directory by returning either '.' or dir (the latter being recommended and used in the readme). A Symbol might make the user's code a little bit easier to read if they do not want to use findUp()'s return value for some reason (i.e. side-effect only usage), but that would be unusual. I suggest waiting to see if anyone requests something like that.

What if the user knows the directory does not exist before reaching root and want to stop? They cannot return a string as that will return that string. I think we either need to support returning a Symbol or throwing a special Error which we catch and handle.

sindresorhus · 2019-04-18T11:09:09Z

Really sorry for dropping the ball on this... I'm just finally able to go through my backlog of PRs to review.

sholladay · 2019-04-19T00:58:28Z

I was thinking the user could just return directory whenever they want to stop early. But then they may need to keep track of whether or not they stopped early, outside of the findUp call and that's not very elegant. So yeah, I guess this is a pretty good use case for a Symbol.

I've added a findUp.stop Symbol that, when returned from the matcher function, stops the search early and returns null. (Naming suggestions welcome.)

# Conflicts: # index.js # package.json # readme.md

index.js

sindresorhus · 2019-04-19T03:27:05Z

readme.md

@@ -47,13 +49,27 @@ 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';


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.

👍

sindresorhus · 2019-04-19T03:27:21Z

Can you update the TS definition for the matcher argument?

sholladay · 2019-04-19T18:54:58Z

index.d.ts

@@ -9,6 +9,8 @@ declare namespace findUp {
 	}
 }

+type Match = string | boolean | null | undefined;


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?

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

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?

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.

Co-Authored-By: sholladay <me@seth-holladay.com>

Changes are ready

sholladay · 2019-04-19T23:57:28Z

This should be ready @sindresorhus. PTAL. :)

index.d.ts

Co-Authored-By: sholladay <me@seth-holladay.com>

* Addition testing for edge cases * Return absolute path if found regardless of cwd * Return undefined for non-existant absolute path * Find nested path in basename of cwd * Tweak variable name

sholladay · 2019-04-24T03:49:56Z

More tests from @coreyfarrell, I've fixed the TypeScript types, and CI is passing.

sindresorhus · 2019-04-24T04:02:51Z

I've fixed the TypeScript types

I don't see that. Did you see my comment in #28 (comment) ?

sholladay · 2019-04-24T15:36:53Z

I was referring to the previous CI failure, which I fixed. Now I've pushed the more strict types based on the review comments.

One note... I wasn't able to use your suggestion for typeof findUp.stop. That was causing these TypeScript errors...

❯ npx tsd

  index.test-d.ts:12:53
  ✖  12:53  Type symbol is not assignable to type string | unique symbol | Promise<Match> | undefined.
  ✖  13:53  Type symbol is not assignable to type string | unique symbol | Promise<Match> | undefined.
  ✖  18:59  Type Promise<symbol> is not assignable to type string | unique symbol | Promise<Match> | undefined.
  Type Promise<symbol> is not assignable to type Promise<Match>.
    Type symbol is not assignable to type Match.
  ✖  19:59  Type Promise<symbol> is not assignable to type string | unique symbol | Promise<Match> | undefined.
  Type Promise<symbol> is not assignable to type Promise<Match>.

  4 errors

I tried just findUp.stop without typeof and that didn't work either. The best I could do is to use symbol. Need more TypeScript fu.

sindresorhus · 2019-04-24T16:55:05Z

@BendingBender Any ideas how we can enforce that only the findup.stop symbol is returned in the Match type? (https://github.com/sindresorhus/find-up/pull/28/files#diff-b52768974e6bc0faccb7d4b75b162c99R10)

sindresorhus · 2019-04-24T16:58:16Z

This is looking great. Thanks for your perseverance on this, @sholladay. 🙌

sindresorhus · 2019-04-24T16:58:35Z

We'll have to resolve #33 before doing a new release, as the next release will be a major one, and #33 changes the behavior.

sholladay · 2019-04-24T17:42:05Z

Wouldn't #33 merely involve new options/methods, making it minor? Or are you thinking that you want to default to true for checking the existence of the path even for matcher functions (in which case I agree it is major compared to current master)?

We could make { exists : false } be the default for findUp(matcher), release it as a minor, and then consider flipping the default later if/when there's another good reason to do a major release.

I'm fine with it either way, but I think personally I lean towards { exists : false } being the default for findUp(matcher) anyway, as it's a bit more flexible and to me that's what matcher is all about: Bring Your Own Algorithm.

sindresorhus · 2019-04-24T17:59:51Z

Wouldn't #33 merely involve new options/methods, making it minor?

No, currently we support both file and directories and we just match by name so the result could any anything. I would like to default to {type: 'file'}, which is a breaking change for people using find-up to find directories.

sindresorhus · 2019-04-24T18:00:54Z

This is not about exists. That's another issue. And I'm fine with keeping matcher not checking for existence. We can add the helper methods later on.

sholladay · 2019-04-24T20:29:17Z

Ah, I forgot you want to default to file until you said it again. Okay, talking about it as a major makes sense then. 👍

I guess in my mind, if we decide that the types must be mutually exclusive (as in, you cannot have the current behavior anymore), then doing it with new methods makes the most sense to me (i.e. findUp.file() and findUp.directory()). It's more explicit and clear.

BendingBender · 2019-04-24T20:44:28Z

I think that this should do the trick. I have however no idea why we have to explicitly annotate the funtion return type. Automatic inference seems not to work here:

diff --git a/index.d.ts b/index.d.ts
index 425e324..5d83ba2 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -1,3 +1,5 @@
+declare const stop: unique symbol;
+
 declare namespace findUp {
        interface Options {
                /**
@@ -8,7 +10,8 @@ declare namespace findUp {
                readonly cwd?: string;
        }
 
-       type Match = string | symbol | undefined;
+       type StopSymbol = typeof stop;
+       type Match = string | StopSymbol | undefined;
 }
 
 declare const findUp: {
@@ -70,7 +73,7 @@ declare const findUp: {
        /**
        Return this in a `matcher` function to stop the search and force `findUp` to immediately return `undefined`.
        */
-       readonly stop: unique symbol;
+       readonly stop: findUp.StopSymbol;
 };
 
 export = findUp;
diff --git a/index.test-d.ts b/index.test-d.ts
index 42b1397..025601e 100644
--- a/index.test-d.ts
+++ b/index.test-d.ts
@@ -9,14 +9,14 @@ expectType<Promise<string | undefined>>(findUp(() => 'unicorn.png'));
 expectType<Promise<string | undefined>>(findUp(() => 'unicorn.png', {cwd: ''}));
 expectType<Promise<string | undefined>>(findUp(() => undefined));
 expectType<Promise<string | undefined>>(findUp(() => undefined, {cwd: ''}));
-expectType<Promise<string | undefined>>(findUp(() => findUp.stop));
-expectType<Promise<string | undefined>>(findUp(() => findUp.stop, {cwd: ''}));
+expectType<Promise<string | undefined>>(findUp((): findUp.StopSymbol => findUp.stop));
+expectType<Promise<string | undefined>>(findUp((): findUp.StopSymbol => findUp.stop, {cwd: ''}));
 expectType<Promise<string | undefined>>(findUp(async () => 'unicorn.png'));
 expectType<Promise<string | undefined>>(findUp(async () => 'unicorn.png', {cwd: ''}));
 expectType<Promise<string | undefined>>(findUp(async () => undefined));
 expectType<Promise<string | undefined>>(findUp(async () => undefined, {cwd: ''}));
-expectType<Promise<string | undefined>>(findUp(async () => findUp.stop));
-expectType<Promise<string | undefined>>(findUp(async () => findUp.stop, {cwd: ''}));
+expectType<Promise<string | undefined>>(findUp(async (): Promise<findUp.StopSymbol> => findUp.stop));
+expectType<Promise<string | undefined>>(findUp(async (): Promise<findUp.StopSymbol> => findUp.stop, {cwd: ''}));
 
 expectType<string | undefined>(findUp.sync('unicorn.png'));
 expectType<string | undefined>(findUp.sync('unicorn.png', {cwd: ''}));
@@ -26,7 +26,7 @@ expectType<string | undefined>(findUp.sync(() => 'unicorn.png'));
 expectType<string | undefined>(findUp.sync(() => 'unicorn.png', {cwd: ''}));
 expectType<string | undefined>(findUp.sync(() => undefined));
 expectType<string | undefined>(findUp.sync(() => undefined, {cwd: ''}));
-expectType<string | undefined>(findUp.sync(() => findUp.stop));
-expectType<string | undefined>(findUp.sync(() => findUp.stop, {cwd: ''}));
+expectType<string | undefined>(findUp.sync((): findUp.StopSymbol => findUp.stop));
+expectType<string | undefined>(findUp.sync((): findUp.StopSymbol => findUp.stop, {cwd: ''}));
 
 expectType<Symbol>(findUp.stop);

Add support for filename matcher function

95d4390

sholladay mentioned this pull request Nov 13, 2018

Add findUpMultiple() method #27

Closed

lakatostamas mentioned this pull request Nov 22, 2018

Feature/find config recursively webpack/webpack-cli#693

Merged

sindresorhus previously requested changes Dec 27, 2018

View reviewed changes

Improve documentation

8b80cf1

sholladay force-pushed the master branch from 0014d29 to d5fabcc Compare December 29, 2018 02:57

Seperate not found tests

e8072bb

sholladay force-pushed the master branch from d5fabcc to e8072bb Compare December 29, 2018 02:58

sholladay added 2 commits December 28, 2018 22:19

Bubble up errors correctly

44e67bf

Fix indentation

102b688

sholladay added 2 commits January 6, 2019 20:12

Merge branch 'master' of github.com:sindresorhus/find-up

7a83413

Fix lint errors

6ad0dd8

sholladay mentioned this pull request Feb 4, 2019

Add ability to specify if looking for file or directory #33

Closed

sholladay mentioned this pull request Apr 18, 2019

Tolerate absolute filenames? #36

Closed

sindresorhus previously requested changes Apr 18, 2019

View reviewed changes

Add Symbol for stopping early

ca48c76

Merge branch 'master' of github.com:sindresorhus/find-up

f858bdd

# Conflicts: # index.js # package.json # readme.md

sindresorhus reviewed Apr 19, 2019

View reviewed changes

sindresorhus mentioned this pull request Apr 19, 2019

Add utility methods to check if the path exists #37

Closed

sholladay added 2 commits April 19, 2019 10:57

Use util.promisify instead of pify

eff8479

Add documentation and TypeScript types

cba6f26

sholladay commented Apr 19, 2019

View reviewed changes

sindresorhus and others added 3 commits April 19, 2019 15:15

Tweak documentation

af9d361

Co-Authored-By: sholladay <me@seth-holladay.com>

Simplify examples

b10d5b2

Merge branch 'master' of github.com:sholladay/find-up

eb97a2f

sholladay requested a review from sindresorhus April 19, 2019 19:48

sholladay changed the title ~~Add support for filename matcher function~~ Add support for path matcher function Apr 20, 2019

sindresorhus reviewed Apr 20, 2019

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

coreyfarrell mentioned this pull request Apr 21, 2019

Add test for finding nested dir at path.basename(cwd) sholladay/find-up#1

Merged

sindresorhus and others added 3 commits April 23, 2019 14:16

Use unique symbol

ae4d8bc

Co-Authored-By: sholladay <me@seth-holladay.com>

Ensure symbol is readonly

bb10d62

Add test for finding nested dir at path.basename(cwd) (#1)

94fafd1

* Addition testing for edge cases * Return absolute path if found regardless of cwd * Return undefined for non-existant absolute path * Find nested path in basename of cwd * Tweak variable name

More strict TypeScript types

41b8bdb

Update index.d.ts

cf3157c

sindresorhus merged commit c0f0dd7 into sindresorhus:master Apr 24, 2019

coreyfarrell mentioned this pull request May 7, 2019

Add ability to specify if looking for file or directory, only match files by default #40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for path matcher function #28

Add support for path matcher function #28

sholladay commented Sep 30, 2018 •

edited

Loading

sholladay commented Oct 7, 2018

sindresorhus Dec 27, 2018

sholladay Dec 29, 2018

sholladay commented Dec 29, 2018

sholladay commented Jan 7, 2019 •

edited

Loading

sindresorhus Apr 18, 2019

sindresorhus Apr 18, 2019

sholladay Apr 18, 2019 •

edited

Loading

sindresorhus Apr 19, 2019

sindresorhus Apr 19, 2019

sindresorhus Apr 18, 2019

sholladay Apr 19, 2019

sindresorhus commented Apr 18, 2019

sindresorhus commented Apr 18, 2019

sholladay commented Apr 19, 2019

sindresorhus Apr 19, 2019

sindresorhus commented Apr 19, 2019 •

edited

Loading

sholladay Apr 19, 2019 •

edited

Loading

sindresorhus Apr 20, 2019

sholladay Apr 20, 2019

sindresorhus Apr 20, 2019 •

edited

Loading

sholladay commented Apr 19, 2019

sholladay commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sholladay commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sholladay commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sholladay commented Apr 24, 2019

BendingBender commented Apr 24, 2019

Add support for path matcher function #28

Add support for path matcher function #28

Conversation

sholladay commented Sep 30, 2018 • edited Loading

sholladay commented Oct 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sholladay commented Dec 29, 2018

sholladay commented Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sholladay Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Apr 18, 2019

sindresorhus commented Apr 18, 2019

sholladay commented Apr 19, 2019

Choose a reason for hiding this comment

sindresorhus commented Apr 19, 2019 • edited Loading

sholladay Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Apr 20, 2019 • edited Loading

Choose a reason for hiding this comment

sholladay commented Apr 19, 2019

sholladay commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sholladay commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sholladay commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sindresorhus commented Apr 24, 2019

sholladay commented Apr 24, 2019

BendingBender commented Apr 24, 2019

sholladay commented Sep 30, 2018 •

edited

Loading

sholladay commented Jan 7, 2019 •

edited

Loading

sholladay Apr 18, 2019 •

edited

Loading

sindresorhus commented Apr 19, 2019 •

edited

Loading

sholladay Apr 19, 2019 •

edited

Loading

sindresorhus Apr 20, 2019 •

edited

Loading