-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
lib: destructuring assignment path
module in fs
#22834
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we confirm that this will land cleanly on 8.x-staging and 6.x-staging? (If so, you can clear this request for changes.) The value here may be less than the cost of backporting the change.
I just confirmed that this has conflicts (with a big diff) on v8-x.staging:
```
diff --cc lib/fs.js
index 8696d38e36,278b66ce8d..0000000000
--- a/lib/fs.js
+++ b/lib/fs.js
@@@ -24,37 -24,83 +24,58 @@@
'use strict'; ++<<<<<<< HEAD
-const { FSReqCallback, statValues } = binding;
-let promisesWarn = true;
|
Can we just land this in the future version ? |
@ZYSzys the problem is even if you backport this, it might interfere with other backports that haven't been done. Unless you want to make sure we're fully caught up on v8.x fs backports. |
Oh, my fault😅. It seems there are such big works need to de done. I'm going to give up this refactor😬. And by the way, do we have the plan to using |
@@ -40,7 +40,7 @@ const { | |||
} = constants; | |||
|
|||
const { _extend } = require('util'); | |||
const pathModule = require('path'); | |||
const { _makeLong, toNamespacedPath, resolve } = require('path'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that moving to this approach does have an impact on the ability to monkeypatch these methods. That, by itself is not a problem, but I believe it does make this PR semver-major
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you mean that we will handle this in the future ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, switching this now is fine, it just means there is a non-zero risk of breaking some code so it has to be treated as a semver-major.
Since it's semver-major
, backporting is a non-issue so I'm dismissing my objection.
@ZYSzys Can you rebase this PR? |
@ZYSzys Sorry, but we need an update using |
Sorry, I simply fixed the conflicts on my phone. |
f7744f5
to
bbb5d2c
Compare
@addaleax Ha, is this OK ? (Sorry, I'm not so export in git😅) |
@ZYSzys Yes, this looks good :) CI: https://ci.nodejs.org/job/node-test-pull-request/17240/ |
@ZYSzys Sorry about this, but can you rebase again? There's one conflict. |
@Trott I think it should be ok now :) |
Minor refactor: Use
destructuring assignment
atpath
module in fs instead of require the whole module.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes