[node] mkdir with recursive: true can have undefined result #50121
Conversation
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 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
Code ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. InactiveThis PR has been inactive for 31 days — it is considered abandoned, and therefore closed! Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 50121,
"author": "xandris",
"headCommitOid": "62ba73d42dae0ed8b893ad42ac70509a14c598f6",
"lastPushDate": "2020-12-15T01:09:57.000Z",
"lastActivityDate": "2020-12-15T09:57:52.000Z",
"maintainerBlessed": false,
"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"
}
],
"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": "a-tarasyuk",
"date": "2020-12-15T09:57:52.000Z",
"isMaintainer": false
},
{
"type": "approved",
"reviewer": "Flarna",
"date": "2020-12-15T09:56:36.000Z",
"isMaintainer": false
}
],
"ciResult": "fail",
"ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/62ba73d42dae0ed8b893ad42ac70509a14c598f6/checks?check_suite_id=1668690392"
} |
|
🔔 @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 — please review this PR in the next few days. Be sure to explicitly select |
|
👋 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.14Comparison details for node/14.14 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. node/v14.14Comparison details for node/14.14 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
|
@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! |
|
It has been more than two weeks and this PR still has no reviews. I'll bump it to the DT maintainer queue. Thank you for your patience, @xandris. (Ping @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.) |
|
@xandris I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed in a week if the issues aren't addressed. |
|
@xandris To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you! |
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), notcb(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.
npm test <package to test>.Select one of these and delete the others:
If changing an existing definition: