fs: simplify copy operations and more cleanups

[BridgeAR]

This is just a minor refactoring to reduce the code overhead.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/27913/

[BridgeAR]

I noticed late that there's actually a test added in #635 that checks that inherited properties are checked as well as fs option (this will likely not work everywhere but at least in streams).

I decided to remove the failing test. I think it's a good idea not to fall back to the prototype but this seems to be a breaking change.
To all who gave their LGs: please take another look and confirm that.

[Trott]

@nodejs/fs

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2162/ ✔️

[Trott]

Removing the author ready label I added as I realize that since it has surfaced that this is a breaking change, some of the LGTMs should be reiterated (especially from @nodejs/tsc folks).

[BridgeAR]

Rebased due to conflicts.

@jasnell@devnexen@lpinca this is semver-major, which was not known while you gave your LG. It would be great if you could confirm/update your LG.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28420/

[mcollina]

I‘m not sure I understand the breaking change. Can you please articulate it further? What would break?

[BridgeAR]

@mcollina inherited object properties won't be accepted as options anymore. So if you do e.g.: Object.create({ encoding: 'utf8' }), encoding won't be detected anymore and it's just handled as empty object instead.

[mcollina]

I don't think doing a breaking change for a refactoring is worth it in this case.

[BridgeAR]

@mcollina the question is if we want to officially support inherited properties or not. I was quite surprised when I noticed the failure that we allowed something like that. AFAIK we do not have any guarantees of supporting inherited properties in any other API. We partially prevent this actively to prevent attacks based on prototype pollution and there are open PRs to do this even more: #30008 and #30063.

I guess it might be a good for the @nodejs/tsc to weight in if we want to generically support inherited properties or if we should instead actively recommend against relying on any prototype properties while passing through options. I am fine to close this if others want to keep the current behavior.

// cc @watson

[mcollina]

I'm happy with the change as long as it is clearly spelled out and it's a direction we would like the project to go.

[lundibundi]

ping @BridgeAR@mcollina should this be added to tsc-agenda then?

[mcollina]

@lundibundi everybody can add the tsc-agenda tag. May I ask to formulate a question to the tsc when you do so? Are you asking for a vote on this?

My take on the -1 is on the breakage for the sake of a refactoring, which is the description of this PR.

[lundibundi]

I think the question would be (only partially related to this PR):
Should the inherited properties be checked and supported in options object throughout the Node.js APIs? (i.e. fs currently supports fs.createReadStream(rangeFile, Object.create({ autoClose: false }));, fs.createReadStream(fn, Object.create({ encoding: 'utf8' }));, while http.request(Object.create({ ... }), res => {}) will ignore properties in options)

AFAIK none of our APIs provide any explicit guarantees of supporting inherited properties in options object. Quickly looking through our code some of the APIs which don't need to store options object, like EventEmitter support this because they just check for them via options.a. At the same time http.createServer, http.get, http.request, child_process APIs use { ...options } which doesn't take inherited properties into account.

@BridgeAR could you please clarify if the above is correct? (feel free to just edit this comment)

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.