Skip to content
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

fix(#201): typings #202

Merged
merged 6 commits into from
Oct 7, 2024
Merged

Conversation

KillerCodeMonkey
Copy link
Contributor

@KillerCodeMonkey KillerCodeMonkey commented Oct 7, 2024

fix:

  • return type of readdirp
  • type of filter options
  • enum for type option, with fallback for old string type option

fix #201

@KillerCodeMonkey KillerCodeMonkey changed the title fix: typings fix(#201): typings Oct 7, 2024
@paulmillr
Copy link
Owner

maybe try const enum instead. We also can't do breaking changes really

@KillerCodeMonkey KillerCodeMonkey changed the title fix(#201): typings fix(#201): typings (breaking change) Oct 7, 2024
@KillerCodeMonkey
Copy link
Contributor Author

why no breaking changes? just release a new major version?

@KillerCodeMonkey
Copy link
Contributor Author

i mean all the not working typings are "breaking" changes...

@paulmillr
Copy link
Owner

just release a new major version

no. I do rare releases. Releasing v5 one month after v4 is unnecessary.

i mean all the not working typings are "breaking" changes...

Yes, so this is a bug fixing, and is OK. For users who did not use readdirpPromise we should not break stuff.

@KillerCodeMonkey KillerCodeMonkey changed the title fix(#201): typings (breaking change) fix(#201): typings Oct 7, 2024
index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
index.ts Show resolved Hide resolved
@paulmillr paulmillr merged commit 66522bd into paulmillr:master Oct 7, 2024
13 checks passed
@KillerCodeMonkey
Copy link
Contributor Author

thanks for the fast response :)

if i had more time, i would get the unit tests up and running again

@paulmillr
Copy link
Owner

@KillerCodeMonkey I have resolved the issue with backwards compat, check out c44446d

@KillerCodeMonkey
Copy link
Contributor Author

sorry some changes are missing there readdirP should return EntryInfo[] and not Path[] what is just a string[].

And in the ReaddirpOptions the root should be optional

@KillerCodeMonkey
Copy link
Contributor Author

KillerCodeMonkey commented Oct 7, 2024

but if i log the result of readdirp it gives me a list of EntryInfo[]... and not a list of strings

So i guess, it should be

export function readdirpPromise(
  root: Path,
  options: Partial<ReaddirpOptions> = {}
): Promise<EntryInfo[]> {
  return new Promise<EntryInfo[]>((resolve, reject) => {
    const files: EntryInfo[] = [];
    readdirp(root, options)
      .on('data', (entry) => files.push(entry))
      .on('end', () => resolve(files))
      .on('error', (error) => reject(error));
  });
}

and the interface for the options, should mark the root option as optional

export type ReaddirpOptions = {
  root?: string;
  fileFilter?: Predicate;
  directoryFilter?: Predicate;
  type?: EntryType;
  lstat?: boolean;
  depth?: number;
  alwaysStat?: boolean;
  highWaterMark?: number;
};

@paulmillr
Copy link
Owner

e658127

@KillerCodeMonkey
Copy link
Contributor Author

nice and now just mark the root option in ReaddirOptions as optional

@paulmillr
Copy link
Owner

paulmillr commented Oct 7, 2024

Partial<something> makes all fields of something optional.

@KillerCodeMonkey
Copy link
Contributor Author

yes

@paulmillr
Copy link
Owner

So it's already there: options: Partial<ReaddirpOptions> = {}

@KillerCodeMonkey
Copy link
Contributor Author

ah then i missed that, sorry

@KillerCodeMonkey
Copy link
Contributor Author

KillerCodeMonkey commented Oct 7, 2024

so then i guess it is ready to ship :).

if you like to release a beta version, let me know. I am here to tests it for my purposes with the promise function

@paulmillr
Copy link
Owner

This is not a huge issue, so I will wait for a month or so before doing 4.0.2 with other fixes.

There is no need to release beta. 1. Users can build their own readdirp from github source. 2. Users can override types using @ts-ignore and similar stuff for now.

@paulmillr
Copy link
Owner

I follow rare release schedule for supply chain security, it makes it easier for users to verify ocassional releases of my software. If, instead, I was spamming "weekly release", it would have been harder to reason about.

@KillerCodeMonkey
Copy link
Contributor Author

yes yes. it was just an idea.

@KillerCodeMonkey
Copy link
Contributor Author

thanks again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong types? result should be EntryInfo[], but is typed as string[]
2 participants