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

feat: add withFileTypes in FileSystem #374

Merged
merged 2 commits into from Apr 14, 2023
Merged

Conversation

fu1996
Copy link
Contributor

@fu1996 fu1996 commented Mar 18, 2023

This pr is from #16780 in the webpack repository. #16780
The relevant unit tests have been run, and types.d.ts have been automatically generated. Please pr this
This is my first attempt to resolve issues in webpack. If there are any issues that do not meet the specification, I will modify them as soon as possible. 😄

Results of running unit tests locally:
image

| "latin1"
| "binary"
| "hex";
withFileTypes?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to pass undefined directly here, so boolean | undefined

types.d.ts Outdated
| "base64"
| "latin1"
| "binary"
| "hex";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a new type BufferEncoding and move them there and reuse here and above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also buffer possible too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for infromation, here is node types for such method:

export function readdir(
        path: PathLike,
        options:
            | {
                  encoding: BufferEncoding | null;
                  withFileTypes?: false | undefined;
              }
            | BufferEncoding
            | undefined
            | null,
        callback: (err: NodeJS.ErrnoException | null, files: string[]) => void
    ): void;
    /**
     * Asynchronous readdir(3) - read a directory.
     * @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
     * @param options The encoding (or an object specifying the encoding), used as the encoding of the result. If not provided, `'utf8'` is used.
     */
    export function readdir(
        path: PathLike,
        options:
            | {
                  encoding: 'buffer';
                  withFileTypes?: false | undefined;
              }
            | 'buffer',
        callback: (err: NodeJS.ErrnoException | null, files: Buffer[]) => void
    ): void;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a new type BufferEncoding and move them there and reuse here and above

"This types.d.ts is automatically generated by running the 'yarn special lint fix' command. I think what I can do is modify the jsDoc syntax in 'Resolver. js' to generate it, but I cannot control the addition of' ascii '|' utf8 '|' utf-8 '|' utf16le '|' ucs2 '|' ucs-2 '|' base64 '|' latin1 '|' binary '|' hex ';"; "Force the use of the BufferEncoding type, even if I manually modify it after running the 'yarn special lint fix' command, but I don't think this is a good method. Do you have any good ideas for controlling the use of the BufferEncoding type instead of its corresponding enumeration value?"?

The following figure shows that after I manually changed to BufferEncoding and executed the 'yarn special lint fix' command, my manual modification was invalid

image

readdir: (
arg0: string,
arg1?:
| null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined possible too

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix some things and we can merge, thank you

@fu1996
Copy link
Contributor Author

fu1996 commented Mar 18, 2023

Let's fix some things and we can merge, thank you

Thank you, cr. It's early morning in China, and I will repair it in 10 hours. 😄

@fu1996
Copy link
Contributor Author

fu1996 commented Mar 19, 2023

Let's fix some things and we can merge, thank you

On the other hand, after I added 'buffer' and undefined and executed the 'yarn special lint fix' command, 'buffer' was successfully automatically generated in the 'types. d.ts' file, but undefined was not generated. For details, see this commit: commit url Is this a normal situation? 😊

@alexander-akait
Copy link
Member

So undefined doesn't generate in types, right?

@fu1996
Copy link
Contributor Author

fu1996 commented Mar 20, 2023

So undefined doesn't generate in types, right?

Yes, undefined cannot be generated by running the command yarn special lint fix. Do you have any good suggestions?

@alexander-akait
Copy link
Member

/cc @TheLarkInn

@TheLarkInn
Copy link
Member

Sorry I'm JUST seeing this! I apologize for the delay @fu1996. This looks awesome thank you so much for the contribution!

@TheLarkInn
Copy link
Member

My understanding is that as long as the parameter is optional that the lint-fix will automatically not generated the undefined also and that the optional parameter satisfies this.

@TheLarkInn
Copy link
Member

I'll wait on confirmation from @alexander-akait on this before merging but all looks good!

@TheLarkInn TheLarkInn merged commit 4efd8e3 into webpack:main Apr 14, 2023
16 checks passed
@TheLarkInn
Copy link
Member

Congrats @fu1996 on your first PR to webpack! 🎉

多谢!!

renovate bot added a commit to Unleash/unleash that referenced this pull request Apr 29, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [enhanced-resolve](https://togithub.com/webpack/enhanced-resolve) |
[`5.12.0` ->
`5.13.0`](https://renovatebot.com/diffs/npm/enhanced-resolve/5.12.0/5.13.0)
|
[![age](https://badges.renovateapi.com/packages/npm/enhanced-resolve/5.13.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/enhanced-resolve/5.13.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/enhanced-resolve/5.13.0/compatibility-slim/5.12.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/enhanced-resolve/5.13.0/confidence-slim/5.12.0)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>webpack/enhanced-resolve</summary>

###
[`v5.13.0`](https://togithub.com/webpack/enhanced-resolve/releases/tag/v5.13.0)

[Compare
Source](https://togithub.com/webpack/enhanced-resolve/compare/v5.12.0...v5.13.0)

#### Features

- Add `withFileTypes` options from FileSystem API by
[@&#8203;fu1996](https://togithub.com/fu1996) in
[webpack/enhanced-resolve#374

#### Bugfixes

- Support wildcards pattern with common suffix in imports/exports field
by [@&#8203;bvanjoi](https://togithub.com/bvanjoi) in
[webpack/enhanced-resolve#353

#### Dependencies

- Bump minimist from 1.2.5 to 1.2.8 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[webpack/enhanced-resolve#373
- Bump json5 from 2.1.3 to 2.2.3 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[webpack/enhanced-resolve#371

#### Developer Experience

- Clean up README example by
[@&#8203;wtlin1228](https://togithub.com/wtlin1228) in
[webpack/enhanced-resolve#366
- Update README to reflect `conditionNames` default by
[@&#8203;43081j](https://togithub.com/43081j) in
[webpack/enhanced-resolve#359

#### New Contributors

- [@&#8203;fu1996](https://togithub.com/fu1996) made their first
contribution in
[webpack/enhanced-resolve#374
- [@&#8203;wtlin1228](https://togithub.com/wtlin1228) made their first
contribution in
[webpack/enhanced-resolve#366
- [@&#8203;43081j](https://togithub.com/43081j) made their first
contribution in
[webpack/enhanced-resolve#359
- [@&#8203;bvanjoi](https://togithub.com/bvanjoi) made their first
contribution in
[webpack/enhanced-resolve#353

**Full Changelog**:
webpack/enhanced-resolve@v5.12.0...v5.13.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/Unleash/unleash).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42My4xIiwidXBkYXRlZEluVmVyIjoiMzUuNjMuMSJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants