Refactor GlobOptions to center option processing.
#170
Conversation
commit: |
|
do we really need to manually validate the function type? doesn't typescript reject non-functions already? also you can delete the additionally, i think doing |
I don't know the structure of
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 |
|
New metrics: Total Shipped Size: Which is now a decrease of |
|
i'm trying to think if there's any other way to write the new types with less duplication, but other than that lgtm |
|
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 export type InternalOptions = GlobOptions & Required<InputOptions>But I'm not sure how this would change the generated types. |
|
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: Also, I managed to find another size reduction in Total Shipped Size: Which is now a decrease of |
|
sorry, i didn't notice your past comment. i'll take a look in a bit! |
|
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 
|
|
Yeah, this reduced the types back to Total Shipped Size: Which is now a decrease of |
| debug: !!process.env.TINYGLOBBY_DEBUG, | ||
| expandDirectories: true, | ||
| followSymbolicLinks: true, | ||
| fs: nativeFs, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/types.ts
Outdated
| export type FileSystemAdapter = Partial<FSLike>; | ||
| // can't use `Matcher` from picomatch as it requires a second argument since @types/picomatch v4 | ||
| export type PartialMatcher = (test: string) => boolean; | ||
| export type GlobInput = string | readonly string[] | Partial<GlobOptions>; |
There was a problem hiding this comment.
i feel like Partial<GlobOptions> should be removed from here (since it's deprecated passing it as the first argument) to avoid encouraging the use of it, also Partial isn't needed anymore after your last changes. also should we export this type to users?
There was a problem hiding this comment.
It's only an internal shortcut type of the current type definition of glob and globSync. Removing it, would cause type errors in handling the legacy code.
You are however right about me forgetting to change it back.
There was a problem hiding this comment.
yeah the issue is that the type might get shown to users when hovering over the functions but users will not be able to import it, unless i'm mistaken and i'm remembering wrong. also by removing it i meant making it string | readonly string[] and then making the first argument of glob like GlobInput | GlobOptions
There was a problem hiding this comment.
Yes, we need GlobOptions to be present and I get several type errors when I remove GlobOptions:
It would be out of scope of this PR to remove the legacy code handling and rather topic of planning version 0.3.0.
There was a problem hiding this comment.
Just tested it: The internal function declaration won't be shown and GlobInput is not generated in the types file.
I only get two overloads by each glob and globSync:
glob(patterns: string | readonly string[], options?: Omit<GlobOptions, "patterns">): Promise<string[]>
glob(options: GlobOptions): Promise<string[]>
globSync(patterns: string | readonly string[], options?: Omit<GlobOptions, "patterns">): string[]
globSync(options: GlobOptions): string[]
Typescript is smart enough to not expose the internal function declaration to not confuse developers.
|
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. |
|
i haven't been doing much code stuff the last few weeks, but i'll let you know if you should change anything! |
2 participants
Goal
The goal of this PR is to center all internal processing of
GlobOptionsto one step of the program and make it type safe for the rest.Why
This change is necessary out of three reasons:
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
GlobOptionswith a truthy default value will now be injected into the options and validated with the user optionsGlobOptionshas been adjusted to assume those options with truthy values are always defined.globandglobSynchave been altered to now showPartial<GlobOtions>instead ofGlobOptionsInternal changes:
patternsAndOptionshas been changed toglobInputwith a new shortcut typeGlobInput.processPatternsandnormalizePatternhave been adjusted to avoid unnecessary property destructuring.normalizeCwdhas been inlinedMetrics
The bundle size has been reduced, but the gzipped size strangely shows an increase (maybe due to build target?)
Before:
After:
Total shipped size before:
14.22KB + 12.92KB + 4.7KB + 4.7KB = 36.54KBTotal shipped size after:
13.86KB + 12.55KB + 4.73KB + 4.73KB = 35.87KBThis 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.