-
-
Notifications
You must be signed in to change notification settings - Fork 527
us006 done #652
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
us006 done #652
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,46 @@ const defaultPlatform: NodeJS.Platform = | |
| process.platform | ||
| : 'linux' | ||
|
|
||
| export const MIN_ASYNC_READDIR_CONCURRENCY = 8 | ||
|
|
||
| const concurrencyMinimumMessage = () => | ||
| `concurrency must be a positive integer greater than or equal to ${MIN_ASYNC_READDIR_CONCURRENCY}` | ||
|
|
||
| const concurrencyReason = | ||
| 'async glob walks need enough concurrent directory reads to avoid starvation in the walker fan-out' | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am dubious of this claim. |
||
|
|
||
| const invalidConcurrencyMessage = () => | ||
| `invalid concurrency option: ${concurrencyMinimumMessage()} because ${concurrencyReason}` | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a method just to return a constant string? just define the string and use that. |
||
|
|
||
| const syncConcurrencyMessage = | ||
| 'concurrency option is not supported for synchronous glob variants' | ||
|
|
||
| export const validateAsyncConcurrency = ( | ||
| concurrency: number | undefined, | ||
| ): number | undefined => { | ||
| if (concurrency === undefined) { | ||
| return undefined | ||
| } | ||
|
|
||
| if ( | ||
| !Number.isInteger(concurrency) || | ||
| concurrency <= 0 || | ||
| concurrency < MIN_ASYNC_READDIR_CONCURRENCY | ||
| ) { | ||
| throw new RangeError(invalidConcurrencyMessage()) | ||
| } | ||
|
|
||
| return concurrency | ||
| } | ||
|
|
||
| export const assertNoSyncConcurrency = ( | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These assertion methods should not be exported if they're only used in this module. Also, these can be tightened up considerably, something like this in const validConcurrency = (concurrency: number | undefined, sync: boolean) =>
concurrency === undefined ? true
: !sync && Number.isInteger(concurrency) && concurrency > 0
const assertValidConcurrency = (concurrency: number | undefined, sync: boolean) => {
if (!validConcurrency) {
throw new Error(
sync ? 'concurrency not supported for sync glob operations'
: `Invalid concurrency value: ${concurrency}`
)
}Then in the various methods: export function globSync(...) {
assertValidConcurrency(options.concurrency, true)
...
}
export function glob(...) {
assertValidConcurrency(options.concurrency, false)
...
} |
||
| concurrency: number | undefined, | ||
| ): void => { | ||
| if (concurrency !== undefined) { | ||
| throw new TypeError(syncConcurrencyMessage) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A `GlobOptions` object may be provided to any of the exported methods, and | ||
| * must be provided to the `Glob` constructor. | ||
|
|
@@ -71,6 +111,17 @@ export interface GlobOptions { | |
| */ | ||
| cwd?: string | URL | ||
|
|
||
| /** | ||
| * Limit simultaneous async directory reads to a positive integer greater | ||
| * than or equal to `8`. Values below `8` are rejected because the async | ||
| * walker fan-out needs enough in-flight `readdir()` work to avoid | ||
| * starvation. | ||
| * | ||
| * Has no effect when omitted, and is not supported by synchronous glob | ||
| * methods. | ||
| */ | ||
| concurrency?: number | ||
|
|
||
| /** | ||
| * Include `.dot` files in normal matches and `globstar` | ||
| * matches. Note that an explicit dot in a portion of the pattern | ||
|
|
@@ -408,6 +459,7 @@ export class Glob<Opts extends GlobOptions> implements GlobOptions { | |
| windowsPathsNoEscape: boolean | ||
| withFileTypes: FileTypes<Opts> | ||
| includeChildMatches: boolean | ||
| concurrency?: number | ||
|
|
||
| /** | ||
| * The options provided to the constructor. | ||
|
|
@@ -455,6 +507,7 @@ export class Glob<Opts extends GlobOptions> implements GlobOptions { | |
| this.realpath = !!opts.realpath | ||
| this.absolute = opts.absolute | ||
| this.includeChildMatches = opts.includeChildMatches !== false | ||
| this.concurrency = opts.concurrency | ||
|
|
||
| this.noglobstar = !!opts.noglobstar | ||
| this.matchBase = !!opts.matchBase | ||
|
|
@@ -558,21 +611,28 @@ export class Glob<Opts extends GlobOptions> implements GlobOptions { | |
| */ | ||
| async walk(): Promise<Results<Opts>> | ||
| async walk(): Promise<(string | Path)[]> { | ||
| const concurrency = validateAsyncConcurrency(this.concurrency) | ||
| const walker = new GlobWalker(this.patterns, this.scurry.cwd, { | ||
| ...this.opts, | ||
| maxDepth: | ||
| this.maxDepth !== Infinity ? | ||
| this.maxDepth + this.scurry.cwd.depth() | ||
| : Infinity, | ||
| platform: this.platform, | ||
| nocase: this.nocase, | ||
| includeChildMatches: this.includeChildMatches, | ||
| }) | ||
|
|
||
| // Walkers always return array of Path objects, so we just have to | ||
| // coerce them into the right shape. It will have already called | ||
| // realpath() if the option was set to do so, so we know that's cached. | ||
| // start out knowing the cwd, at least | ||
| return [ | ||
| ...(await new GlobWalker(this.patterns, this.scurry.cwd, { | ||
| ...this.opts, | ||
| maxDepth: | ||
| this.maxDepth !== Infinity ? | ||
| this.maxDepth + this.scurry.cwd.depth() | ||
| : Infinity, | ||
| platform: this.platform, | ||
| nocase: this.nocase, | ||
| includeChildMatches: this.includeChildMatches, | ||
| }).walk()), | ||
| ...(await ( | ||
| concurrency === undefined ? | ||
| walker.walk() | ||
| : walker.walkWithConcurrency(concurrency) | ||
| )), | ||
| ] | ||
| } | ||
|
|
||
|
|
@@ -581,6 +641,7 @@ export class Glob<Opts extends GlobOptions> implements GlobOptions { | |
| */ | ||
| walkSync(): Results<Opts> | ||
| walkSync(): (string | Path)[] { | ||
| assertNoSyncConcurrency(this.concurrency) | ||
| return [ | ||
| ...new GlobWalker(this.patterns, this.scurry.cwd, { | ||
| ...this.opts, | ||
|
|
@@ -600,7 +661,8 @@ export class Glob<Opts extends GlobOptions> implements GlobOptions { | |
| */ | ||
| stream(): Minipass<Result<Opts>, Result<Opts>> | ||
| stream(): Minipass<string | Path, string | Path> { | ||
| return new GlobStream(this.patterns, this.scurry.cwd, { | ||
| const concurrency = validateAsyncConcurrency(this.concurrency) | ||
| const stream = new GlobStream(this.patterns, this.scurry.cwd, { | ||
| ...this.opts, | ||
| maxDepth: | ||
| this.maxDepth !== Infinity ? | ||
|
|
@@ -609,14 +671,18 @@ export class Glob<Opts extends GlobOptions> implements GlobOptions { | |
| platform: this.platform, | ||
| nocase: this.nocase, | ||
| includeChildMatches: this.includeChildMatches, | ||
| }).stream() | ||
| }) | ||
| return concurrency === undefined ? | ||
| stream.stream() | ||
| : stream.streamWithConcurrency(concurrency) | ||
| } | ||
|
|
||
| /** | ||
| * Stream results synchronously. | ||
| */ | ||
| streamSync(): Minipass<Result<Opts>, Result<Opts>> | ||
| streamSync(): Minipass<string | Path, string | Path> { | ||
| assertNoSyncConcurrency(this.concurrency) | ||
| return new GlobStream(this.patterns, this.scurry.cwd, { | ||
| ...this.opts, | ||
| maxDepth: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ import type { | |
| GlobOptionsWithFileTypesTrue, | ||
| GlobOptionsWithFileTypesUnset, | ||
| } from './glob.js' | ||
| import { Glob } from './glob.js' | ||
| import { assertNoSyncConcurrency, Glob } from './glob.js' | ||
| import { hasMagic } from './has-magic.js' | ||
|
|
||
| export { escape, unescape } from 'minimatch' | ||
|
|
@@ -55,6 +55,7 @@ export function globStreamSync( | |
| pattern: string | string[], | ||
| options: GlobOptions = {}, | ||
| ) { | ||
| assertNoSyncConcurrency(options.concurrency) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary? Won't it assert in the constructor anyway? |
||
| return new Glob(pattern, options).streamSync() | ||
| } | ||
|
|
||
|
|
@@ -108,6 +109,7 @@ export function globSync( | |
| pattern: string | string[], | ||
| options: GlobOptions = {}, | ||
| ) { | ||
| assertNoSyncConcurrency(options.concurrency) | ||
| return new Glob(pattern, options).walkSync() | ||
| } | ||
|
|
||
|
|
@@ -163,6 +165,7 @@ export function globIterateSync( | |
| pattern: string | string[], | ||
| options: GlobOptions = {}, | ||
| ) { | ||
| assertNoSyncConcurrency(options.concurrency) | ||
| return new Glob(pattern, options).iterateSync() | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this limited to 8? Feels arbitrary. What happens when it's lower?
There should really be no reason why it can't be set at 1, in fact; it'd just mean that each readdir waits for the one before it to complete before proceeding. Would be slow, of course, but also very memory and CPU efficient.