[node] mkdir with recursive: true can have undefined result #50721

xandris · 2021-01-19T23:35:43Z

The CI is blocked by microsoft/TypeScript#42337

If all subpaths of the given path already exist, the result is not EEXIST like the non-recursive case, but undefined. The callback is called like cb(null), not cb(null, undefined).

I'm a little concerned this is undefined behavior because the API documentation doesn't say what's supposed to happen if all path components already exist, other than that it isn't considered an error like the non-recursive case. But it would stand to reason that if it's not an error, and no directory was created, then the "first directory created" is literally undefined.

But even though the official API docs don't mention it, you can't in practice rely on the return value to be defined. I believe type definitions shouldn't incorporate implementation bugs, but I also don't believe this is an implementation bug, just a documentation oversight.

This seems like the only reasonable definition given the circumstances; that is, if the Node developers eventually address this edge case, I struggle to see what the alternative would be.

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/api/fs.html#fs_fs_mkdir_path_options_callback (kind of)

typescript-bot · 2021-01-19T23:36:18Z

@xandris Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

node on npm, on unpkg
- owner-approval: @eps1lon, @Flarna

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": 50721,
  "author": "xandris",
  "headCommitOid": "71fee5b780d9749d62252e60408f439f7bcef3af",
  "lastPushDate": "2021-02-03T05:52:14.000Z",
  "lastActivityDate": "2021-02-03T22:38:15.000Z",
  "maintainerBlessed": false,
  "mergeOfferDate": "2021-02-03T22:27:31.000Z",
  "mergeRequestDate": "2021-02-03T22:38:15.000Z",
  "mergeRequestUser": "xandris",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "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/v13/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v13/test/fs.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "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"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "amcasey",
      "date": "2021-02-03T22:26:27.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "eps1lon",
      "date": "2021-02-03T08:38:46.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "Flarna",
      "date": "2021-02-03T07:46:59.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "a-tarasyuk",
      "date": "2021-01-20T18:13:21.000Z",
      "abbrOid": "62ba73d"
    }
  ],
  "ciResult": "pass"
}

typescript-bot · 2021-01-19T23:36:19Z

🔔 @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 — 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 · 2021-01-19T23:36:20Z

@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-01-19T23:52:41Z

👋 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	#50721	diff
Batch compilation
Memory usage (MiB)	112.5	113.5	+0.8%
Type count	17887	17887	0%
Assignability cache size	5343	5343	0%

Language service
Samples taken	28	28	0%
Identifiers in tests	28	28	0%
`getCompletionsAtPosition`
Mean duration (ms)	584.2	577.6	-1.1%
Mean CV	11.1%	10.6%
Worst duration (ms)	720.7	664.3	-7.8%
Worst identifier	preopens	env
`getQuickInfoAtPosition`
Mean duration (ms)	586.9	576.9	-1.7%
Mean CV	12.8%	11.1%
Worst duration (ms)	653.7	630.1	-3.6%
Worst identifier	instance	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	#50721	diff
Batch compilation
Memory usage (MiB)	113.3	110.0	-2.9%
Type count	17887	17887	0%
Assignability cache size	5343	5343	0%

Language service
Samples taken	28	28	0%
Identifiers in tests	28	28	0%
`getCompletionsAtPosition`
Mean duration (ms)	576.0	579.9	+0.7%
Mean CV	12.1%	12.0%
Worst duration (ms)	639.8	721.6	+12.8%
Worst identifier	env	process
`getQuickInfoAtPosition`
Mean duration (ms)	574.9	575.1	0.0%
Mean CV	10.1%	9.8%
Worst duration (ms)	640.2	661.9	+3.4%
Worst identifier	instance	env

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

Flarna · 2021-01-20T08:02:51Z

Is this the same as #50121?

xandris · 2021-01-20T16:38:45Z

Is this the same as #50121?

Yes but that one got closed for inactivity before it got merged. I think it didn't get merged because there were unrelated CI failures. It looks like the latest 4.2 is causing problems on a few packages. I'll take a look at CI failures again and try to itemize their causes

eps1lon · 2021-01-20T17:42:46Z

just a documentation oversight.

I think it's somewhat implied by

if recursive is true, the first directory path created, (err, [path]).

-- https://nodejs.org/api/fs.html#fs_fs_mkdir_path_options_callback

If no directory is created then no path can be given. Though ideally, API documentation is as explicit as possible so I opened nodejs/node#37010.

xandris · 2021-01-22T00:15:54Z

Once microsoft/TypeScript#42447 gets merged, I think the CI error will finally go away.

typescript-bot · 2021-02-03T05: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!

If all subpaths of the given path already exist, the result is not EEXIST like the non-recursive case, but undefined. The callback receives only one argument (the Error, null).

typescript-bot · 2021-02-03T06:17:17Z

@a-tarasyuk, @Flarna Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot · 2021-02-03T07:47:37Z

@a-tarasyuk Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

amcasey · 2021-02-03T22:26:56Z

@xandris Thanks for the thoughtful explanation! Sorry it didn't go through the first time.

xandris · 2021-02-03T22:38:15Z

Ready to merge

xandris · 2021-02-03T22:38:54Z

It has been a journey!

typescript-bot · 2021-02-03T22:51:26Z

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

typescript-bot · 2021-02-03T22:51:53Z

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

@xandris

…n have undefined result by @xandris If all subpaths of the given path already exist, the result is not EEXIST like the non-recursive case, but undefined. The callback receives only one argument (the Error, null).

@xandris

…n have undefined result by @xandris If all subpaths of the given path already exist, the result is not EEXIST like the non-recursive case, but undefined. The callback receives only one argument (the Error, null).

typescript-bot added The CI failed When GH Actions fails Critical package labels Jan 19, 2021

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jan 19, 2021

Flarna approved these changes Jan 20, 2021

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Jan 20, 2021

eps1lon mentioned this pull request Jan 20, 2021

fs.mkdir: Add explicit note about undefined path when recursive nodejs/node#37010

Closed

a-tarasyuk approved these changes Jan 20, 2021

View reviewed changes

typescript-bot removed the Owner Approved A listed owner of this package signed off on the pull request. label Feb 3, 2021

[node] mkdir with recursive: true can have undefined result

71fee5b

If all subpaths of the given path already exist, the result is not EEXIST like the non-recursive case, but undefined. The callback receives only one argument (the Error, null).

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

Flarna approved these changes Feb 3, 2021

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Feb 3, 2021

eps1lon approved these changes Feb 3, 2021

View reviewed changes

amcasey approved these changes Feb 3, 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 labels Feb 3, 2021

typescript-bot merged commit e6bba82 into DefinitelyTyped:master Feb 3, 2021

[node] mkdir with recursive: true can have undefined result #50721

[node] mkdir with recursive: true can have undefined result #50721

Uh oh!

Conversation

xandris commented Jan 19, 2021

Uh oh!

typescript-bot commented Jan 19, 2021 • 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 Jan 19, 2021

Uh oh!

typescript-bot commented Jan 19, 2021

Uh oh!

typescript-bot commented Jan 19, 2021

node/v14.14

node/v14.14

Uh oh!

Flarna commented Jan 20, 2021

Uh oh!

xandris commented Jan 20, 2021

Uh oh!

eps1lon commented Jan 20, 2021

Uh oh!

xandris commented Jan 22, 2021

Uh oh!

typescript-bot commented Feb 3, 2021

Uh oh!

typescript-bot commented Feb 3, 2021

Uh oh!

typescript-bot commented Feb 3, 2021

Uh oh!

amcasey commented Feb 3, 2021

Uh oh!

xandris commented Feb 3, 2021

Uh oh!

xandris commented Feb 3, 2021

Uh oh!

typescript-bot commented Feb 3, 2021

Uh oh!

typescript-bot commented Feb 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

typescript-bot commented Jan 19, 2021 •

edited

Loading