fix: add missing null on file watch event filename by sosoba · Pull Request #65505 · DefinitelyTyped/DefinitelyTyped

sosoba · 2023-05-16T03:55:20Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://nodejs.org/dist/latest-v18.x/docs/api/fs.html#filename-argument
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

typescript-bot · 2023-05-16T03:56:02Z

@sosoba Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

node — on npm, on unpkg
- owner-approval: @mcollina, @Semigradsky

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Only a DT maintainer can approve changes without tests

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 65505,
  "author": "sosoba",
  "headCommitOid": "4ab7de1a7a4446009929c06c038857dadb1beb45",
  "mergeBaseOid": "d9244492f8e3e1cf1560185d8c02b4a75a18633b",
  "lastPushDate": "2023-05-18T05:46:56.000Z",
  "lastActivityDate": "2023-05-26T20:10:56.000Z",
  "mergeOfferDate": "2023-05-25T18:00:42.000Z",
  "mergeRequestDate": "2023-05-26T20:10:56.000Z",
  "mergeRequestUser": "Semigradsky",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/fs/promises.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/fs/promises.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/fs/promises.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/ts4.8/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/ts4.8/fs/promises.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/fs/promises.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/ts4.8/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/ts4.8/fs/promises.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "mcollina",
      "date": "2023-05-26T14:42:32.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2023-05-25T17:59:59.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "Semigradsky",
      "date": "2023-05-18T08:18:03.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1548940598,
  "ciResult": "pass"
}

typescript-bot · 2023-05-16T03:56:04Z

Hey @sosoba,

😒 Your PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Please consider adding tests to cover the change you're making. Including tests allows this PR to be merged by yourself and the owners of this module.

This can potentially save days of time for you!

typescript-bot · 2023-05-16T03:56:05Z

🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina @Semigradsky — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Semigradsky · 2023-05-16T08:16:50Z

@sosoda It looks like filename in FileChangeInfo (node:fs/promises module) can be null too?

All the `caveats` for `fs.watch()` also apply to `fsPromises.watch()`

sosoba · 2023-05-16T09:01:57Z

@sosoda It looks like filename in FileChangeInfo (node:fs/promises module) can be null too?

You have right!. Doc says:

All the caveats for fs.watch() also apply to fsPromises.watch()

I added a second change.

Semigradsky · 2023-05-16T11:44:19Z

Related: nodejs/node#48032

peterblazejewicz · 2023-05-16T18:55:10Z

what's the outcome here? Do we need TS4.8 update and ship?

Semigradsky · 2023-05-16T19:58:00Z

Sure! @sosoba please, add these changes also to another applicable node versions (v14, v16, v18 folders) and to TypeScript<4.8 versions (ts4.8 folders).

Semigradsky

👍

jakebailey · 2023-05-18T17:09:32Z

I feel like I'm missing something in the docs; the link provided in the PR description doesn't seem to go anywhere. Everything I'm reading seems to imply that this filename is not nullable: https://nodejs.org/dist/latest-v18.x/docs/api/fs.html#event-change

jakebailey · 2023-05-18T17:13:14Z

Ugh, it was my browser. This section appears in the fs.watch block:

Providing filename argument in the callback is only supported on Linux, macOS, Windows, and AIX. Even on supported platforms, filename is not always guaranteed to be provided. Therefore, don't assume that filename argument is always provided in the callback, and have some fallback logic if it is null.

This is going to be pretty breaky :\

jakebailey · 2023-05-18T17:14:19Z

I know that this is probably technically correct, but is it super useful if all of the major OSs definitely get a string every time?

sosoba · 2023-05-19T04:13:22Z

I know that this is probably technically correct, but is it super useful if all of the major OSs definitely get a string every time?

What you mean "major OS"? I have issue with empty filename for Windows 11. This happens ex. when Node watch which files are modified by another Java process.

jakebailey · 2023-05-19T04:15:45Z

I guess the wording doesn't preclude a "supported" OS passing null...

typescript-bot · 2023-05-22T06:46:00Z

⏳ Hi @sosoba,

It's been a few days since this PR was approved by Semigradsky and we're waiting for a DT maintainer to give a review.

If you would like to short-circuit this wait, you can edit some of the test files in the package which verify how the .d.ts files work. This would allow the PR to be merged by you or the DT module owners after a re-review.

jakebailey

I guess we'll see; I know TS itself will have to fix a type error with this.

typescript-bot · 2023-05-25T18:00:42Z

@sosoba: Everything looks good here. I am ready to merge this PR (at 4ab7de1) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@microsoft, @DefinitelyTyped, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @n-e, @galkin, @parambirs, @eps1lon, @SimonSchick, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky: you can do this too.)

mcollina

lgtm

Semigradsky · 2023-05-26T20:10:56Z

Ready to merge

@sosoba

…event filename by @sosoba * fix: add missing null on watch event filename * fix: add missing null on fsPromises.watch result filename All the `caveats` for `fs.watch()` also apply to `fsPromises.watch()` * fix: applly fs.watch to another node versions

Fixed the type of `fs.watch`, which may pass to `null` for the callback (DefinitelyTyped/DefinitelyTyped#65505). Removed `WritableStream` type, since its events don't match `busboy` anymore (DefinitelyTyped/DefinitelyTyped#68313).

fix: add missing null on watch event filename

5d2b030

sosoba requested review from eps1lon and peterblazejewicz as code owners May 16, 2023 03:55

typescript-bot added Critical package Untested Change This PR does not touch tests labels May 16, 2023

fix: add missing null on fsPromises.watch result filename

37a82f0

All the `caveats` for `fs.watch()` also apply to `fsPromises.watch()`

fix: applly fs.watch to another node versions

4ab7de1

Semigradsky approved these changes May 18, 2023

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label May 18, 2023

jakebailey approved these changes May 25, 2023

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels May 25, 2023

mcollina approved these changes May 26, 2023

View reviewed changes

typescript-bot merged commit 04a94ba into DefinitelyTyped:master May 26, 2023

jakebailey mentioned this pull request May 26, 2023

Fix fs.watch callback type for new @types/node microsoft/TypeScript#54417

Merged

ghost mentioned this pull request May 28, 2023

doc: fix filename type in watch result nodejs/node#48032

Merged

Conversation

sosoba commented May 16, 2023

Uh oh!

typescript-bot commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented May 16, 2023

Uh oh!

typescript-bot commented May 16, 2023

Uh oh!

Semigradsky commented May 16, 2023

Uh oh!

sosoba commented May 16, 2023

Uh oh!

Semigradsky commented May 16, 2023

Uh oh!

peterblazejewicz commented May 16, 2023

Uh oh!

Semigradsky commented May 16, 2023

Uh oh!

Semigradsky left a comment

Choose a reason for hiding this comment

Uh oh!

jakebailey commented May 18, 2023

Uh oh!

jakebailey commented May 18, 2023

Uh oh!

jakebailey commented May 18, 2023

Uh oh!

sosoba commented May 19, 2023

Uh oh!

jakebailey commented May 19, 2023

Uh oh!

typescript-bot commented May 22, 2023

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented May 25, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Semigradsky commented May 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

typescript-bot commented May 16, 2023 •

edited

Loading