[node] bigint options for fstat and lstat #50120

xandris · 2020-12-14T23:25:33Z

According to the Node Filesystem API, these methods all support an opts parameter that allows it to return bigint stats:

stat
lstat
fstat

They support the bigint option in the following modes:

Callback
Synchronous
util.promisify
fs.promises

Each call signature can handle these arguments:

No extra argument yields Stats
Passing undefined yields Stats
Passing an empty object yields Stats
Passing { bigint: false } yields Stats
Passing { bigint: true } yields BigIntStats

The work was already done for stat and statSync, so this PR just extends that to lstat and fstat async and sync variants and makes the call signatures a bit more liberal.

I switched the call signatures from using the BigIntOptions type to using types like StatOptions & { bigint: true } where needed. This is the approach already taken by mkdir to handle the recursive option, for example. Also, it's acceptable to pass in an empty options object, so I made StatOptions.bigint optional.

fs.promises.fstat doesn't appear to have ever existed. (I looked at all the Node JS API documents going back to version 10.) This PR removes it. This functionality is implemented by FileHandle.stat.

These definitions go back to Node 10, but the v10 and v11 variants didn't have the other definitions for bigint stats, so I didn't touch those.

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>.

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://nodejs.org/docs/latest-v14.x/api/fs.html#fs_fs_fstatsync_fd_options

typescript-bot · 2020-12-14T23:26:12Z

@xandris 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

Code Reviews

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

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by a DT maintainer

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": 50120,
  "author": "xandris",
  "headCommitOid": "1e39b21b87ab4fe6bc6d43739fa5cbcde9f801ec",
  "lastPushDate": "2021-02-03T06:23:08.000Z",
  "lastActivityDate": "2021-02-19T00:03:57.000Z",
  "maintainerBlessed": false,
  "mergeOfferDate": "2021-02-18T16:28:25.000Z",
  "mergeRequestDate": "2021-02-19T00:03:57.000Z",
  "mergeRequestUser": "xandris",
  "hasMergeConflict": false,
  "isFirstContribution": 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/test/fs.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v12/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v13/fs.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "a-tarasyuk",
        "alvis",
        "r3nya",
        "btoueg",
        "brunoscheufler",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "KSXGitHub",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "j-oliveras",
        "bhongy",
        "chyzwar",
        "trivikr",
        "nguymin4",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "Ryan-Willpower",
        "peterblazejewicz",
        "addaleax",
        "JasonHK",
        "victorperin",
        "ZYSzys"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "orta",
      "date": "2021-02-18T16:27:45.000Z",
      "isMaintainer": true
    }
  ],
  "ciResult": "pass"
}

typescript-bot · 2020-12-14T23:26:13Z

🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @ExE-Boss @Ryan-Willpower @peterblazejewicz @addaleax @JasonHK @victorperin @ZYSzys — 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.

typescript-bot · 2020-12-14T23:50:00Z

@xandris The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-12-14T23:51:06Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

node/v14.14

Comparison details for node/14.14 📊

	master	#50120	diff
Batch compilation
Memory usage (MiB)	110.9	108.2	-2.4%
Type count	17683	17687	0%
Assignability cache size	5062	5062	0%

Language service
Samples taken	28	28	0%
Identifiers in tests	28	28	0%
`getCompletionsAtPosition`
Mean duration (ms)	648.8	646.2	-0.4%
Mean CV	10.2%	11.3%
Worst duration (ms)	754.7	760.9	+0.8%
Worst identifier	preopens	preopens
`getQuickInfoAtPosition`
Mean duration (ms)	643.8	633.7	-1.6%
Mean CV	10.3%	9.6%
Worst duration (ms)	703.9	695.9	-1.1%
Worst identifier	wasm	WebAssembly

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

node/v14.14

Comparison details for node/14.14 📊

	master	#50120	diff
Batch compilation
Memory usage (MiB)	103.3	107.9	+4.5%
Type count	17683	17687	0%
Assignability cache size	5062	5062	0%

Language service
Samples taken	28	28	0%
Identifiers in tests	28	28	0%
`getCompletionsAtPosition`
Mean duration (ms)	643.1	646.2	+0.5%
Mean CV	10.8%	8.4%
Worst duration (ms)	737.5	787.9	+6.8%
Worst identifier	wasm	preopens
`getQuickInfoAtPosition`
Mean duration (ms)	635.0	617.0	-2.8%
Mean CV	10.5%	8.9%
Worst duration (ms)	731.9	697.6	-4.7%
Worst identifier	preopens	preopens

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

node/v14.14

Comparison details for node/14.14 📊

	master	#50120	diff
Batch compilation
Memory usage (MiB)	113.9	109.1	-4.2%
Type count	17683	17687	0%
Assignability cache size	5062	5062	0%

Language service
Samples taken	28	28	0%
Identifiers in tests	28	28	0%
`getCompletionsAtPosition`
Mean duration (ms)	642.9	636.1	-1.1%
Mean CV	10.6%	9.9%
Worst duration (ms)	723.3	742.5	+2.6%
Worst identifier	wasi	instance
`getQuickInfoAtPosition`
Mean duration (ms)	636.2	635.9	-0.1%
Mean CV	9.5%	9.8%
Worst duration (ms)	708.0	713.2	+0.7%
Worst identifier	returnOnExit	importObject

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

xandris · 2020-12-15T01:57:12Z

Half baked, closing for now

typescript-bot · 2020-12-15T03:46:12Z

@xandris The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

xandris · 2020-12-15T04:16:15Z

I don't know what's going on in the CI build but I don't *think* I broke it! 😥

xandris · 2020-12-15T17:59:20Z

Okay, last tweak incoming. The inference should support these cases:

If you know the options are defined and bigint is true, then you'll always get BigIntStats
If the options might be defined, but would have bigint undefined or false, then you'll always get Stats
If the options might be defined, but you don't know anything about them, you can get either Stats or BigIntStats
If the options are undefined, you'll always get Stats.

Based on this playground I think I need 3 overloads for opts in the final position, and a fourth for the callback case where the opts in the middle of the parameter list. So, almost there, just need to make the bigint: false case with an optional param and put the false case first. I think I was imagining some case where TypeScript would automatically deduce the general case from the false and true cases, but it's probably a good thing it doesn't!

typescript-bot · 2020-12-15T19:27:41Z

@xandris The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2021-02-03T05:28:50Z

@xandris The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

amcasey · 2021-02-03T23:05:59Z

Re-pinging owners now that the CI is green and the code has stabilized

@microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @ExE-Boss @Ryan-Willpower @peterblazejewicz @addaleax @JasonHK @victorperin @ZYSzys

Flarna · 2021-02-04T14:51:59Z

types/node/fs.d.ts

    export function stat(path: PathLike, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
+    export function stat(path: PathLike, options: StatOptions & { bigint?: false } | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
+    export function stat(path: PathLike, options: StatOptions & { bigint: true }, callback: (err: NodeJS.ErrnoException | null, stats: BigIntStats) => void): void;
+    export function stat(path: PathLike, options: StatOptions | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats | BigIntStats) => void): void;


I think the options === undefined case is not optimal here as the result should be of type Stats.

I think omitting | undefined here is better because this case is handled correct by the overload two lines above.

Its possible you could have a weakly defined StatOptions | undefined that you want to pass in, which should be legal. Since you can't differentiate between whether its a Stats or BigIntStats due to the weaker type, I would expect that to be the result. This definition looks fine to me.

Also, the undefined case should hit the first overload (with & { bigint?: false }), which will correctly return Stats.

Flarna · 2021-02-04T14:53:41Z

types/node/fs.d.ts

    export function stat(path: PathLike, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
+    export function stat(path: PathLike, options: StatOptions & { bigint?: false } | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
+    export function stat(path: PathLike, options: StatOptions & { bigint: true }, callback: (err: NodeJS.ErrnoException | null, stats: BigIntStats) => void): void;


what is the advantage of StatOptions & { bigint: true } compared to BigIntOptions used till now?

It actually looks easier to see why this matters with the options inline.

Flarna · 2021-02-04T14:56:11Z

types/node/fs.d.ts

    export function stat(path: PathLike, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
+    export function stat(path: PathLike, options: StatOptions & { bigint?: false } | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;


There is quite some copy paste of this compound type.
What about adding something like this instead:

interface NoBigIntOptions { bigint?: false; }

It actually looks easier to see why this matters with the options inline. Are you recommending it be StatOptions & NoBigIntOptions? Or are you recommending a NoBigIntOptions interface that inherits from StatOptions? If the latter, The naming of BigIntOptions/NoBigIntOptions seems very ambiguous to me. Personally, I'd rather see either:

StatOptions & { bigint: true } (as is written), or

BigIntStatOptions/NoBigIntStatOptions (indicating the specific variations of StatOptions

Personally I think (1) is easier to read and reason over.

rbuckton · 2021-02-11T06:39:26Z

types/node/fs.d.ts


    // NOTE: This namespace provides design-time support for util.promisify. Exported members do not exist at runtime.
    export namespace stat {
        /**
         * Asynchronous stat(2) - Get file status.
         * @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
         */
-        function __promisify__(path: PathLike, options: BigIntOptions): Promise<BigIntStats>;


Should BigIntOptions be deprecated since it's not referenced in the API anymore?

rbuckton · 2021-02-12T01:33:55Z

@Flarna do you have any other comments or feedback on this one? If not, I'll go ahead and approve.

typescript-bot · 2021-02-14T06:59:19Z

Re-ping @microsoft, @DefinitelyTyped, @jkomyno, @a-tarasyuk, @alvis, @r3nya, @btoueg, @BrunoScheufler, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @KSXGitHub, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @n-e, @galkin, @parambirs, @eps1lon, @SimonSchick, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @j-oliveras, @bhongy, @chyzwar, @trivikr, @nguymin4, @yoursunny, @qwelias, @ExE-Boss, @Ryan-Willpower, @peterblazejewicz, @addaleax, @JasonHK, @victorperin, @ZYSzys:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

Flarna · 2021-02-14T19:18:09Z

@Flarna do you have any other comments or feedback on this one? If not, I'll go ahead and approve.

@rbuckton No more comments.

orta

Giving Ron's approval, as this looks right to me after a read through of the discussion

typescript-bot · 2021-02-18T16:28:25Z

@xandris Everything looks good here. Great job! I am ready to merge this PR (at 1e39b21) on your behalf.

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, @a-tarasyuk, @alvis, @r3nya, @btoueg, @BrunoScheufler, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @KSXGitHub, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @n-e, @galkin, @parambirs, @eps1lon, @SimonSchick, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @j-oliveras, @bhongy, @chyzwar, @trivikr, @nguymin4, @yoursunny, @qwelias, @ExE-Boss, @Ryan-Willpower, @peterblazejewicz, @addaleax, @JasonHK, @victorperin, @ZYSzys: you can do this too.)

xandris · 2021-02-19T00:03:57Z

Ready to merge

typescript-bot · 2021-02-19T00:41:11Z

I just published @types/node@14.14.30 to npm.

typescript-bot · 2021-02-19T00:41:33Z

I just published @types/node@13.13.44 to npm.

typescript-bot · 2021-02-19T00:41:54Z

I just published @types/node@12.20.3 to npm.

@xandris

…lstat by @xandris * [node] bigint options for fstat and lstat * [node] fixes for stat, fstat, lstat in all modes Changes the overloads to be more in line with the mkdir recursive API by consistently using StatOptions, making the bigint field optional, and using { bigint: true } and { bigint?: false } cases to discriminate between Stats and BigIntStats where possible. Also fs.promises.fstat doesn't seem to have ever existed. * [node] more stat/lstat/fstat overload tweaks, more tests * node: fix some copypasta in v12

@xandris

…lstat by @xandris * [node] bigint options for fstat and lstat * [node] fixes for stat, fstat, lstat in all modes Changes the overloads to be more in line with the mkdir recursive API by consistently using StatOptions, making the bigint field optional, and using { bigint: true } and { bigint?: false } cases to discriminate between Stats and BigIntStats where possible. Also fs.promises.fstat doesn't seem to have ever existed. * [node] more stat/lstat/fstat overload tweaks, more tests * node: fix some copypasta in v12

typescript-bot added Critical package Untested Change This PR does not touch tests labels Dec 14, 2020

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Dec 14, 2020

typescript-bot added the The CI failed When GH Actions fails label Dec 14, 2020

typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Dec 14, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Dec 14, 2020

xandris closed this Dec 15, 2020

typescript-bot removed this from Needs Author Action in New Pull Request Status Board Dec 15, 2020

xandris reopened this Dec 15, 2020

typescript-bot removed The CI failed When GH Actions fails Untested Change This PR does not touch tests labels Dec 15, 2020

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Dec 15, 2020

typescript-bot added the The CI failed When GH Actions fails label Dec 15, 2020

typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Dec 15, 2020

typescript-bot removed the The CI failed When GH Actions fails label Dec 15, 2020

typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Dec 15, 2020

xandris mentioned this pull request Dec 15, 2020

[Bug] fs patch doesn't handle { bigint: true } yarnpkg/berry#2232

Closed

1 task

typescript-bot added the The CI failed When GH Actions fails label Dec 15, 2020

typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Dec 15, 2020

typescript-bot added Where is GH Actions? GH Actions didn't give a response to this PR Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Jan 4, 2021

typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Feb 3, 2021

node: fix some copypasta in v12

1e39b21

typescript-bot removed the The CI failed When GH Actions fails label Feb 3, 2021

typescript-bot moved this from Needs Author Action to Needs Maintainer Review in New Pull Request Status Board Feb 3, 2021

Flarna reviewed Feb 4, 2021

View reviewed changes

rbuckton reviewed Feb 11, 2021

View reviewed changes

typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Feb 14, 2021

Flarna mentioned this pull request Feb 15, 2021

[@types/node] BigIntOptions overloads don't exist on fs/promises typings #51229

Closed

orta approved these changes Feb 18, 2021

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Feb 18, 2021

typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Feb 18, 2021

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Feb 19, 2021

typescript-bot merged commit 5967146 into DefinitelyTyped:master Feb 19, 2021

typescript-bot removed this from Recently Merged in New Pull Request Status Board Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[node] bigint options for fstat and lstat #50120

[node] bigint options for fstat and lstat #50120

xandris commented Dec 14, 2020 •

edited

typescript-bot commented Dec 14, 2020 •

edited

typescript-bot commented Dec 14, 2020 •

edited

typescript-bot commented Dec 14, 2020

typescript-bot commented Dec 14, 2020

xandris commented Dec 15, 2020

typescript-bot commented Dec 15, 2020

xandris commented Dec 15, 2020 via email •

edited

xandris commented Dec 15, 2020

typescript-bot commented Dec 15, 2020

typescript-bot commented Feb 3, 2021

amcasey commented Feb 3, 2021

Flarna Feb 4, 2021 •

edited

rbuckton Feb 11, 2021

rbuckton Feb 11, 2021

Flarna Feb 4, 2021

rbuckton Feb 11, 2021 •

edited

Flarna Feb 4, 2021

rbuckton Feb 11, 2021

rbuckton Feb 11, 2021

rbuckton commented Feb 12, 2021

typescript-bot commented Feb 14, 2021 •

edited

Flarna commented Feb 14, 2021

orta left a comment

typescript-bot commented Feb 18, 2021

xandris commented Feb 19, 2021

typescript-bot commented Feb 19, 2021

typescript-bot commented Feb 19, 2021

typescript-bot commented Feb 19, 2021

		export function stat(path: PathLike, callback: (err: NodeJS.ErrnoException \| null, stats: Stats) => void): void;
		export function stat(path: PathLike, options: StatOptions & { bigint?: false } \| undefined, callback: (err: NodeJS.ErrnoException \| null, stats: Stats) => void): void;

[node] bigint options for fstat and lstat #50120

[node] bigint options for fstat and lstat #50120

Conversation

xandris commented Dec 14, 2020 • edited

typescript-bot commented Dec 14, 2020 • edited

1 package in this PR

Code Reviews

Status

typescript-bot commented Dec 14, 2020 • edited

typescript-bot commented Dec 14, 2020

typescript-bot commented Dec 14, 2020

node/v14.14

node/v14.14

node/v14.14

xandris commented Dec 15, 2020

typescript-bot commented Dec 15, 2020

xandris commented Dec 15, 2020 via email • edited

xandris commented Dec 15, 2020

typescript-bot commented Dec 15, 2020

typescript-bot commented Feb 3, 2021

amcasey commented Feb 3, 2021

Flarna Feb 4, 2021 • edited

Choose a reason for hiding this comment

rbuckton Feb 11, 2021

Choose a reason for hiding this comment

rbuckton Feb 11, 2021

Choose a reason for hiding this comment

Flarna Feb 4, 2021

Choose a reason for hiding this comment

rbuckton Feb 11, 2021 • edited

Choose a reason for hiding this comment

Flarna Feb 4, 2021

Choose a reason for hiding this comment

rbuckton Feb 11, 2021

Choose a reason for hiding this comment

rbuckton Feb 11, 2021

Choose a reason for hiding this comment

rbuckton commented Feb 12, 2021

typescript-bot commented Feb 14, 2021 • edited

Flarna commented Feb 14, 2021

orta left a comment

Choose a reason for hiding this comment

typescript-bot commented Feb 18, 2021

xandris commented Feb 19, 2021

typescript-bot commented Feb 19, 2021

typescript-bot commented Feb 19, 2021

typescript-bot commented Feb 19, 2021

xandris commented Dec 14, 2020 •

edited

typescript-bot commented Dec 14, 2020 •

edited

typescript-bot commented Dec 14, 2020 •

edited

xandris commented Dec 15, 2020 via email •

edited

Flarna Feb 4, 2021 •

edited

rbuckton Feb 11, 2021 •

edited

typescript-bot commented Feb 14, 2021 •

edited