Refactor GlobOptions to center option processing.

[Torathion]

Goal

The goal of this PR is to center all internal processing of GlobOptions to one step of the program and make it type safe for the rest.

Why

This change is necessary out of three reasons:

• Reducing duplicate code
• Easing maintenance and future updates
• Removing duplicate type checks of options

Currently, options are processed when needed, which cause multiple shape recalculations (more JS engine work) and can cause issues if steps need to be refactored. It improves code quality and readability.

What has changed

• GlobOptions with a truthy default value will now be injected into the options and validated with the user options
• The type definition of GlobOptions has been adjusted to assume those options with truthy values are always defined.
• The type definition of glob and globSync have been altered to now show Partial<GlobOtions> instead of GlobOptions

Internal changes:

• patternsAndOptions has been changed to globInput with a new shortcut type GlobInput.
• Function definition of processPatterns and normalizePattern have been adjusted to avoid unnecessary property destructuring.
• normalizeCwd has been inlined

Metrics

The bundle size has been reduced, but the gzipped size strangely shows an increase (maybe due to build target?)

Before:

ℹ [CJS] dist\index.cjs      14.22 kB │ gzip: 4.32 kB
ℹ [CJS] dist\index.cjs.map  26.15 kB │ gzip: 7.76 kB
ℹ [CJS] 2 files, total: 40.37 kB
ℹ [ESM] dist\index.mjs      12.92 kB │ gzip: 3.85 kB
ℹ [ESM] dist\index.mjs.map  26.09 kB │ gzip: 7.73 kB
ℹ [ESM] dist\index.d.mts     4.70 kB │ gzip: 1.62 kB
ℹ [ESM] 3 files, total: 43.71 kB
ℹ [CJS] dist\index.d.cts  4.70 kB │ gzip: 1.62 kB
ℹ [CJS] 1 files, total: 4.70 kB

After:

ℹ [CJS] dist\index.cjs      13.86 kB │ gzip: 4.44 kB
ℹ [CJS] dist\index.cjs.map  26.08 kB │ gzip: 8.08 kB
ℹ [CJS] 2 files, total: 39.94 kB
ℹ [ESM] dist\index.mjs      12.55 kB │ gzip: 3.98 kB
ℹ [ESM] dist\index.mjs.map  26.02 kB │ gzip: 8.03 kB
ℹ [ESM] dist\index.d.mts     4.73 kB │ gzip: 1.62 kB
ℹ [ESM] 3 files, total: 43.30 kB
ℹ [CJS] dist\index.d.cts  4.73 kB │ gzip: 1.62 kB
ℹ [CJS] 1 files, total: 4.73 kB

Total shipped size before: 14.22KB + 12.92KB + 4.7KB + 4.7KB = 36.54KB
Total shipped size after: 13.86KB + 12.55KB + 4.73KB + 4.73KB = 35.87KB

This is a bundle size reduction of 0.67KB (1.8%) and saves 22.1GB of weekly traffic.

---

This PR is a split part of the closed PR #88, which grew too much in scope and changes.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/tinyglobby@170

commit: 0b61932

[SuperchupuDev]

do we really need to manually validate the function type? doesn't typescript reject non-functions already? also you can delete the debug option from process.env test since with this pr it is no longer a necessary test for 100% coverage :D

additionally, i think doing Required<GlobOptions> where needed and leaving GlobOptions's properties all optional is a better idea, since users who import GlobOptions might have some issues otherwise

[Torathion]

do we really need to manually validate the function type? doesn't typescript reject non-functions already?

I don't know the structure of FSLike or FileSystemAdapter, so I imagined you could still add primitive or objects as properties.

additionally, i think doing Required where needed and leaving GlobOptions's properties all optional is a better idea, since users who import GlobOptions might have some issues otherwise

Good point, but I have to see how I have to handle that as some falsy values are still set as undefined. If I use Required, even these values need to be injected. My first idea is to add a type like InternalOptions and declare some falsy properties as optional.

[Torathion]

New metrics:

ℹ [CJS] dist\index.cjs      13.84 kB │ gzip: 4.43 kB
ℹ [CJS] dist\index.cjs.map  25.97 kB │ gzip: 8.06 kB
ℹ [CJS] 2 files, total: 39.81 kB
ℹ [ESM] dist\index.mjs      12.53 kB │ gzip: 3.97 kB
ℹ [ESM] dist\index.mjs.map  25.91 kB │ gzip: 8.01 kB
ℹ [ESM] dist\index.d.mts     4.70 kB │ gzip: 1.62 kB
ℹ [ESM] 3 files, total: 43.14 kB
ℹ [CJS] dist\index.d.cts  4.70 kB │ gzip: 1.62 kB
ℹ [CJS] 1 files, total: 4.70 kB

Total Shipped Size: 13.84 kB + 12.53 kB + 4.70 kB + 4.70 kB = 35.77 kB

Which is now a decrease of 0.77 KB or 2.1% which saves 25.48GB of weekly traffic (33.1 million weekly downloads).

[SuperchupuDev]

i'm trying to think if there's any other way to write the new types with less duplication, but other than that lgtm

[Torathion]

That's the thing. I don't like the way this is written either. If you could just do:

export interface InternalGlobOptions extends Required<GlobOptions> {
  declare absolute: boolean
}

Then it would be fine.

The other way I can think of now is to create a common subset type GlobOptions, make a type with the optional parameters InputOptions and then write for InternalOptions:

export type InternalOptions = GlobOptions & Required<InputOptions>

But I'm not sure how this would change the generated types.

[Torathion]

Since I didn't receive any feedback on that idea, I implemented for review. This should be cleaner and every property is now documented when hovered over, but increases the output bundle:

ℹ [CJS] dist\index.cjs      13.83 kB │ gzip: 4.44 kB
ℹ [CJS] dist\index.cjs.map  25.96 kB │ gzip: 8.06 kB
ℹ [CJS] 2 files, total: 39.79 kB
ℹ [ESM] dist\index.mjs      12.52 kB │ gzip: 3.98 kB
ℹ [ESM] dist\index.mjs.map  25.90 kB │ gzip: 8.01 kB
ℹ [ESM] dist\index.d.mts     4.75 kB │ gzip: 1.64 kB
ℹ [ESM] 3 files, total: 43.16 kB
ℹ [CJS] dist\index.d.cts  4.75 kB │ gzip: 1.64 kB
ℹ [CJS] 1 files, total: 4.75 kB

Also, I managed to find another size reduction in index.ts. New Metrics:

Total Shipped Size: 13.83 kB + 12.52 kB + 4.75 kB + 4.75 kB = 35.85 kB

Which is now a decrease of 0.69 KB or 1.89% which saves 22.84GB of weekly traffic (33.1 million weekly downloads).

[SuperchupuDev]

sorry, i didn't notice your past comment. i'll take a look in a bit!

[SuperchupuDev]

what about something like this? that way we can keep all the options under the same interface. it wouldn't affect the user-facing types since the InternalOptions type is never exposed to users and the Options type would remain as-is

[Torathion]

Yeah, this reduced the types back to 4.70 KB and keeps the documentation as well. Depends on how it looks to you. New Metrics:

ℹ [CJS] dist\index.cjs      13.83 kB │ gzip: 4.44 kB
ℹ [CJS] dist\index.cjs.map  25.96 kB │ gzip: 8.06 kB
ℹ [CJS] 2 files, total: 39.79 kB
ℹ [ESM] dist\index.mjs      12.52 kB │ gzip: 3.98 kB
ℹ [ESM] dist\index.mjs.map  25.90 kB │ gzip: 8.01 kB
ℹ [ESM] dist\index.d.mts     4.70 kB │ gzip: 1.62 kB
ℹ [ESM] 3 files, total: 43.12 kB
ℹ [CJS] dist\index.d.cts  4.70 kB │ gzip: 1.62 kB
ℹ [CJS] 1 files, total: 4.70 kB

Total Shipped Size: 13.83 kB + 12.52 kB + 4.70 kB + 4.70 kB = 35.75 kB

Which is now a decrease of 0.79 KB or 2.16% which saves 26.15GB of weekly traffic (33.1 million weekly downloads).

[SuperchupuDev]

looks a lot better now, thank you! one more thing and this should be ready to merge

[SuperchupuDev]

i have no perf metrics regarding fs imports and it wouldn't really affect tinyglobby performance, but wouldn't this be too expensive since it would have to import every export of the fs module? also would likely make tree-shaking harder

[Torathion]

That's hard to answer question what tree-shakes better. After scouring through Node.JS docs and reading a few articles, I still don't have a clear answer, but your method is technically more tree-shakable than my method. The main performance impact comes from the extra start-up time, which will be less the deeper it is integrated into another tools also importing node:fs.

Since tree-shaking happens during source-code parsing, I don't think either of our methods would make it entirely tree-shakable, since it is uncertain wether or not options.fs is set or not, but your method only restricts itself to those 6 functions, which makes it more efficient.

I think the best method of ensuring we only import the things we need and tree-shake the rest, as node built-in modules are almost always treated as external packages, is to use destructured imports:

import { readdir, readdirSync, realpath, realpathSync, stat, statSync } from 'node:fs'

const DefaultFS = { readdir, readdirSync, realpath, realpathSync, stat, statSync }

But since tinyglobby is more like a core utility in huge node.js tools, the overall effect would be minimal either way.

[Torathion]

If there is anything else that needs to be done, instruct me what I should change. From my part, it looks fine and we can wrap this up, so I can move to the next split part of the original PR.

[SuperchupuDev]

i haven't been doing much code stuff the last few weeks, but i'll let you know if you should change anything!

[SuperchupuDev]

finally have some time to code and review stuff. all mostly looks good!! i think the only remaining thing to do is to refactor the nativeFs stuff to allow for tree-shaking. also, i've given it some thought and i think it'd be best if the fs option was left undefined by default, so that fdir can handle the default methods. the reason for this is future-proofing in case fdir starts using more fs methods. if we left this as-is and fdir did that, it'd be likely it would break usage of the library that doesn't use the fs option

[Torathion]

I'm on board to say "nuke that shit!" and remove everything about it including FileSystemAdapter. While it is a comfort utility to allow partial overrides, the fs override is probably intended to make fdir be able to crawl in special file system environments like in-memory and people would only considering this option when inside those special environments.

Since the tests only test simple, partial overrides, I deleted them as well. Since we only propagate this option, I hope this won't affect coverage.

[Torathion]

Metrics of discarded commit:

ℹ [CJS] dist\index.cjs      13.49 kB │ gzip: 4.32 kB
ℹ [CJS] dist\index.cjs.map  25.29 kB │ gzip: 7.80 kB
ℹ [CJS] 2 files, total: 38.78 kB
ℹ [ESM] dist\index.mjs      12.21 kB │ gzip: 3.86 kB
ℹ [ESM] dist\index.mjs.map  25.25 kB │ gzip: 7.77 kB
ℹ [ESM] dist\index.d.mts     4.65 kB │ gzip: 1.59 kB
ℹ [ESM] 3 files, total: 42.11 kB
ℹ [CJS] dist\index.d.cts  4.65 kB │ gzip: 1.59 kB
ℹ [CJS] 1 files, total: 4.65 kB

Total Shipped Size: 13.49 KB + 12.21 KB + 4.65 KB + 4.65 KB = 35 KB

Which is now a decrease of 1.54 KB or 4.21% which saves 50.97 GB of weekly traffic (33.1 million weekly downloads).

[SuperchupuDev]

sadly, we need to keep our custom fs handling, since fdir's handling works in a different way than fast-glob (fast-glob allows you to only provide some fs methods, fdir doesn't), i just meant that we should leave it undefined as default

[Torathion]

I've removed the commit that removed the fs handling and instead made it undefined by default. It actually removed like 20 bytes from the final shipped size, totaling at:

0.81 KB or 2.21% which saves 26.81 GB of weekly traffic (33.1 million weekly downloads).

[SuperchupuDev]

coverage lowered a bit, i think restoring the fs tests should fix it? everything else is good, i can merge once that's done 👍

[Torathion]

It seems like the attempt to revert the commit only partially worked or did nothing at all. I've added the tests back in.

[SuperchupuDev]

thank you!! sorry for the wait