doc: deprecate coercion to string in fs.write, fs.writeFile, fs.appendFile

[LiviaMedeiros]

Refs: #41677

This deprecation allows to align Callback/fs.write with Synchronous and Promise cousin methods, and to implement options parameter for it.

Let me know if doc-only phase might be skipped for this one.

[aduh95]

Doc-only deprecation makes perfect sense here, so it can be backported to LTS release lines.

The very string argument for deprecate this is that @@toPrimitive (even if inherited) has "higher priority" over toString (even if own):

const fs = require('fs');
class Parent {
  [Symbol.toPrimitive](){ return '@@toPrimitive'; }
}
fs.writeFileSync('./test.txt', new class extends Parent {constructor(){
  super();
  this.toString = () => 'toString';
}});
console.log(fs.readFileSync('./test.txt', 'utf-8') === '@@toPrimitive'); // true

[aduh95]

/cc @ljharb since you're the author of #34993.

[ljharb]

I’m fine with a docs-only deprecation, but i don’t think Symbol.toPrimitive being higher than toString is an argument in favor of it - the deprecation is phrased to apply to coercion, which applies to both Symbol.toPrimitive and a toString method.

Anyone passing a stringifiable object into fs.readFile is surely intending to do so.

[aduh95]

but i don’t think Symbol.toPrimitive being higher than toString is an argument in favor of it - the deprecation is phrased to apply to coercion, which applies to both Symbol.toPrimitive and a toString method.

To clarify what I meant by that, the problem I have with the current API is that it checks Object.hasOwn(bufferOrOption, 'toString'), but then don't even call that toString method if there is a @@toPrimitive in the prototype chain. It's at best inconsistent. Deprecating the coercion is to remove that footgun.

[LiviaMedeiros]

@aduh95 thanks a lot for this code example!

First of all, my bad, I completely overlooked the fact that methods such as fs.writeFile and fs.appendFile rely on fs.write and therefore inherit its behaviour. I'll list them in this deprecation and doublecheck cousin methods in Promises/Synchronous APIs.

Also initially I thought that typechecking in these methods is transparent, but apparently it is not.
Instead of passing its second param "as is", fs.writeFileSync converts it to a string, then converts that string to Buffer, then calls fs.writeSync with guaranteed ArrayBufferView instance.
While if we try to call fs.writeSync with "object with own toString property" directly, the outcome is abortion.

Callback API version works in the same way, except fs.write doesn't abort. Therefore, it's a subject for deprecation procedure.

fsPromises.writeFile does similar thing, but doesn't convert the object to string before passing it to Buffer.from, so it's a guaranteed ERR_INVALID_ARG_TYPE; i.e. coercion removal in Promises API is not a breaking change.

[ljharb]

@aduh95 that's totally fair, and it'd also be fine imo to improve that detection with Symbol.toPrimitive in bufferOrOption.

[aduh95]

@LiviaMedeiros Shouldn't we also deprecate coercion in fs.writeFileSync and fs.appendFileSync?

[LiviaMedeiros]

Squashed and force-pushed to edit first commit message, sorry for the mess.

@aduh95 yep, doublechecked and added those. Some coercions aren't documented.
Since "File" methods don't depend on typechecks inside fs.writeSync, it seems to be safe to adjust this method while "File" ones are still in deprecation phase.

@ljharb thanks for feedback!
Most of context-specific reasons are covered by #41666 and #41677 (tl;dr: stringable objects are obstacle blocking "named arguments" feature)
However, I'd like to clarify more generic arguments for it as well.
Also refs: #31030 as initial version of this.

In my opinion, it is always better to keep param checking strict, simple and unambiguous - in both code and docs.
typeof param === 'string' does the job. ArrayBuffer.isView(param) does.
Does typeof param === 'string' || typeof param === 'object' && param !== null && typeof param.toString === 'function' && ObjectPrototypeHasOwnProperty(param, 'toString') (which must be called after checking for isArrayBufferView first)?

Anyone passing a stringifiable object into fs.readFile is surely intending to do so.

I'm not sure about it. Anyone might pass a value returned via multiple layers of abstraction and third-party libraries. And if they care, they will explicitly typecheck and/or stringify it in userland.
And if an error happens (e.g. someone somewhere refactored toString to @@toPrimitive; or something failed and returned false), it's better to throw than to corrupt files by writing results of implicit coercion.

Also, "stringifiable object" is a complicated term. Is new String('a') or null or {} stringifiable? "has own callable property toString" doesn't sound like a common thing in JS.

Idiomatic JS behaviour (like how USVString from w3c specs works) is a well-known footgun feature, most users are familiar with it and know what to expect. And even unconditional coercion could be completely fine in function like fs.writeAsString(fd, data, ...), where data must be interpreted as nothing but string.
But fs.write is polymorphic, so we have no choice but to strictly segregate types; since otherwise we couldn't choose an obvious and unambiguous path to process the data.

Most of above are generalizations, and I understand that "has own toString" rule was intended to reach the balance and solve that problem. It is also a clever workaround to keep compatibility with legacy code for some extra time. Unfortunately, attempts to improve this way might make the logic even more obscure and error-prone.

it'd also be fine imo to improve that detection with Symbol.toPrimitive in bufferOrOption.

[ // even builtin objects are inconsistent now
  new Date(),                     // false true
  new String('amString'),         // false false
  new URL('https://example.com'), // false false
].forEach(s => console.log(s.hasOwnProperty('toString'), Symbol.toPrimitive in s));

// how do we explain this in docs?
// and why do we allow implicit/inherited @@toPrimitive while toString has to be ownProperty?
class Potato { [Symbol.toPrimitive](hint) { if (hint === 'number') return 42; } }
const potato = new Potato();
potato.toString = () => 'amString';
console.log(String(potato)); // "undefined"

// what if this will evaluate to true in future standards?
Symbol.toPrimitive in {};

// what if user did this?
Array.prototype[Symbol.toPrimitive] = function() { return this.join('\n'); }

I hope this makes the overall idea more clear. If any of these thoughts are wrong, I'll be happy to learn from mistakes.

[ljharb]

@LiviaMedeiros in general, I agree that strictly validating input is preferred. However, landing #34993 unbroke a use case I consider important.

I do not think such breakage is "worth it" merely to give a nicer argument signature; if that's the goal, I think a better choice may be an entirely new API.

[nodejs-github-bot]

Landed in 4b198ff