buffer: make File cloneable

[KhafraDev]

Fixes: #47612

[KhafraDev]

Since I had to move back to using symbols instead of private properties for brand checking, the error for invalid this has changed, so I'm marking it as semver-major.

[KhafraDev]

cc @anonrig @aduh95 @nodejs/buffer

[nodejs-github-bot]

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

[anonrig]

I still think we should find an elegant solution to this issue, and not revert to using symbols.

[aduh95]

I still think we should find an elegant solution to this issue, and not revert to using symbols.

Would you really prefer that the File objects are never made cloneable, hurting web compat? I don't think it's fair to block this PR on an implementation detail (especially when it was already addressed in #47613 (comment)), if you feel strongly about this, you should open a concurrent PR, or propose a working solution.

[aduh95]

LGTM, I gave it a go and try to use private fields, I didn't manage to get anything working, using a Symbol seems the correct option for now.

I'm not sure it's actually a semver-major change, as it's still going to be a TypeError, we're adding a code property not modifying it, and the error message is not covered by semver rules in our policy.

Can you add an entry in the history section to track what version has cloneable File implementation?

node/doc/api/buffer.md

Lines 5065 to 5074 in eb17645

	## Class: `File`
	<!-- YAML
	added:
	- v19.2.0
	- v18.13.0
	changes:
	- version: v20.0.0
	pr-url: https://github.com/nodejs/node/pull/47153
	description: No longer experimental.

[anonrig]

Would you really prefer that the File objects are never made cloneable, hurting web compat? I don't think it's fair to block this PR on an implementation detail (especially when it was already addressed in #47613 (comment)), if you feel strongly about this, you should open a concurrent PR, or propose a working solution.

@aduh95 I agree and share your concerns, and I am, too, interested in a web-compatible File class, but unfortunately, this pull request didn't receive any attention from people who have more in depth knowledge on this, and I prefer to wait for them to look into it (This is my main reason for blocking this pull request). The decision to revert and use Symbols affect all classes inside the Node project. Whether or not we are using private properties, by such limitations, should be at least discussed with people who initially implemented them in case we are missing something here. Unfortunately, I don't know the correct person to tag this, and I kindly ask for your help reaching out to them.

Unfortunately, I can't look into this issue in the next few days, but I'll try to look as soon as possible. If you think we should proceed with the current approach or in a time-sensitive manner, we can always add tsc-agenda, which will create a way for TSC to discuss the approach and might help us find a common ground here.

[aduh95]

@anonrig here's what the collaborator guide says about objections:

node/doc/contributing/collaborator-guide.md

Lines 153 to 156 in 6c76d7e

	objection. Any pull request objection must include a clear reason for that
	objection, and the objector must remain responsive for further discussion
	towards consensus about the direction of the pull request. Where possible,
	provide a set of actionable steps alongside the objection.

If you plan on being irresponsive in the following days, blocking that PR is really not a good timing.

The decision to revert and use Symbols affect all classes inside the Node project.

That's simply not true, this PR only changes the File class, there's no reason other parts of the codebase would be affected.

Whether or not we are using private properties, by such limitations, should be at least discussed with people who initially implemented them in case we are missing something here

Fair enough, that's not a reason for blocking the PR though, and Khafra did ping the buffer team already. If this PR lands, that doesn't mean the implementation will be frozen, someone can open a follow up PR.

IMO getting closer to web compat is more important for our users than the symbol vs private fields debate that have no effect for them; if you disagree that's fine of course but then please say so explicitly, it's always better to have thorough requests for change.

Dismissing due to being unavailable in the next couple of days and respecting the collaborator guide.

[Trott]

FWIW, something I sometimes do is leave a request-for-changes with a comment saying something like "Once X and Y happen, feel free to dismiss this request-for-change. You don't need to ping me or anything."

[nodejs-github-bot]

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

[Trott]

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

@KhafraDev Longstanding annoyance with our CI is that it does not handle merge commits well. Can you rebase/force-push to this branch to get rid of the merge commit?

[KhafraDev]

Thanks! Will keep that in mind for the future :)

[nodejs-github-bot]

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

[aduh95]

@nodejs/tsc since this is semver-major

[nodejs-github-bot]

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

[benjamingr]

This would likely be faster not going through dictionary mode (e.g. extract it to a FileState class)

[KhafraDev]

I don't have much time to look into the test failures. If anyone else wants to take it over that would be great.