feat: support custom fs #136
Conversation
| @@ -25,13 +26,23 @@ export type PathsOutput = string[]; | |||
|
|
|||
| export type Output = OnlyCountsOutput | PathsOutput | GroupOutput; | |||
|
|
|||
| export type FSLike = { | |||
There was a problem hiding this comment.
this is purposely explicit rather than just FSLike = typeof nativeFs since the fs module has all sorts of exports (both types and actual exports) which we don't want to have to stub out to satisfy the type when bringing our own fs-like object
|
was about to start working on this, then saw you had already implemented and pr'd it!! thanks a lot. hopefully @thecodrr can review soon 🙏 |
|
This is awesome! |
|
it truly is |
| import type { Dirent } from "fs"; | ||
| import * as nativeFs from "fs"; |
There was a problem hiding this comment.
just a style question probably for @thecodrr, but I wonder if these two lines should be combined into a single line
|
I noticed there aren't any docs being updated as part of this PR. That would probably be nice to add |
|
this probably needs a rebase so that it can get formatted with prettier 👀 |
|
i have rebased and run the formatter @thecodrr can you take another look when you get time? |
|
@43081j it looks like this needs a rebase again 👀 |
| @@ -10,7 +10,8 @@ import * as resolveSymlink from "./functions/resolve-symlink"; | |||
| import * as invokeCallback from "./functions/invoke-callback"; | |||
| import * as walkDirectory from "./functions/walk-directory"; | |||
| import { Queue } from "./queue"; | |||
| import { Dirent } from "fs"; | |||
| import type { Dirent } from "fs"; | |||
| import * as nativeFs from "fs"; | |||
There was a problem hiding this comment.
We can't import fs anywhere in the project (importing the types are okay since they are stripped). It needs to be imported conditionally otherwise anyone trying to use fdir outside node won't be able to use it - defeating the whole point of this PR.
Perhaps we can use require but if we generate esm builds, that won't work afaik. await import is also not an option since build options are synchronous.
There was a problem hiding this comment.
we already have this problem - the walker imports path
are we sure that's the point of this PR? afaik nobody is expecting to run fdir in browsers (which is the only place that doesn't have path and fs, since all node-like runtimes provide them)
if we want to avoid depending on any node built ins, its a bigger job since we'd need to pull in pathe or something (agnostic path library) instead of path
@SuperchupuDev what does tinyglobby need this for?
There was a problem hiding this comment.
We can't import
fsanywhere in the project (importing the types are okay since they are stripped). It needs to be imported conditionally otherwise anyone trying to usefdiroutsidenodewon't be able to use it - defeating the whole point of this PR.Perhaps we can use
requirebut if we generateesmbuilds, that won't work afaik.await importis also not an option since build options are synchronous.
we can try to use require for conditionally importing it like we already do with picomatch. tsdown (the tool thats used in the dual build pr) automatically defines a require function with import { createRequire } from 'node:module'; createRequire(import.meta.url) for esm builds
There was a problem hiding this comment.
we already have this problem - the walker imports
pathare we sure that's the point of this PR? afaik nobody is expecting to run
fdirin browsers (which is the only place that doesn't havepathandfs, since all node-like runtimes provide them)if we want to avoid depending on any node built ins, its a bigger job since we'd need to pull in
patheor something (agnostic path library) instead ofpath@SuperchupuDev what does tinyglobby need this for?
iirc the library that requested the feature wants it so that they can use when globbing their custom fs wrapper that does caching
There was a problem hiding this comment.
yeah lets not go down a rabbit hole trying to hack in some require calls 😅
fdir currently relies on Node libraries (it imports path)
we can do this with fs too and satisfy the feature request with the PR in its current state afaict
nobody can use fdir in a browser right now without bundling it. so we haven't changed that
let me know if im missing something though
There was a problem hiding this comment.
i agree, not using require would be ideal
|
@43081j can you resolve the conflicts so I can merge this? |
Adds a new `fs` option for specifying your own `fs` implementation.
|
should be all good now 👍 |
Adds a new
fsoption for specifying your ownfsimplementation.Fixes #134