From 950a4411facdcaf6450cf3943f034177d0e21e3d Mon Sep 17 00:00:00 2001 From: Livia Medeiros Date: Mon, 12 Sep 2022 18:29:43 +0900 Subject: [PATCH] fs: remove coercion to string in writing methods Moves DEP0162 to End-of-Life. PR-URL: https://github.com/nodejs/node/pull/42796 Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- doc/api/deprecations.md | 5 +++- doc/api/fs.md | 22 +++++++++++++---- lib/fs.js | 31 ++++++------------------ lib/internal/fs/promises.js | 6 ++--- lib/internal/fs/utils.js | 23 ------------------ test/parallel/test-fs-write-file-sync.js | 30 +++++++++++------------ test/parallel/test-fs-write.js | 25 ++++--------------- 7 files changed, 51 insertions(+), 91 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index ac42905c7024d1..fd2d7a8c331919 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3131,6 +3131,9 @@ resources and not the actual references. -Type: Runtime +Type: End-of-Life Implicit coercion of objects with own `toString` property, passed as second parameter in [`fs.write()`][], [`fs.writeFile()`][], [`fs.appendFile()`][], diff --git a/doc/api/fs.md b/doc/api/fs.md index 316b1b30111e06..b47ce5f4c817d4 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -4534,6 +4534,10 @@ default with the above values. * `fd` {integer} -* `string` {string|Object} +* `string` {string} * `position` {integer|null} **Default:** `null` * `encoding` {string} **Default:** `'utf8'` * `callback` {Function} @@ -4568,8 +4572,8 @@ changes: * `written` {integer} * `string` {string} -Write `string` to the file specified by `fd`. If `string` is not a string, or an -object with an own `toString` function property, then an exception is thrown. +Write `string` to the file specified by `fd`. If `string` is not a string, +an exception is thrown. `position` refers to the offset from the beginning of the file where this data should be written. If `typeof position !== 'number'` the data will be written at @@ -4602,6 +4606,10 @@ details. * `file` {string|Buffer|URL|integer} filename or file descriptor -* `data` {string|Buffer|TypedArray|DataView|Object} +* `data` {string|Buffer|TypedArray|DataView} * `options` {Object|string} * `encoding` {string|null} **Default:** `'utf8'` * `mode` {integer} **Default:** `0o666` @@ -5840,6 +5848,10 @@ this API: [`fs.utimes()`][]. * `file` {string|Buffer|URL|integer} filename or file descriptor -* `data` {string|Buffer|TypedArray|DataView|Object} +* `data` {string|Buffer|TypedArray|DataView} * `options` {Object|string} * `encoding` {string|null} **Default:** `'utf8'` * `mode` {integer} **Default:** `0o666` diff --git a/lib/fs.js b/lib/fs.js index 06aa44b303e9ca..8272fb679ad37f 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -39,7 +39,6 @@ const { ReflectApply, SafeMap, SafeSet, - String, StringPrototypeCharCodeAt, StringPrototypeIndexOf, StringPrototypeSlice, @@ -84,7 +83,6 @@ const { FSReqCallback } = binding; const { toPathIfFileURL } = require('internal/url'); const { customPromisifyArgs: kCustomPromisifyArgsSymbol, - deprecate, kEmptyObject, promisify: { custom: kCustomPromisifiedSymbol, @@ -123,7 +121,6 @@ const { validateRmOptionsSync, validateRmdirOptions, validateStringAfterArrayBufferView, - validatePrimitiveStringAfterArrayBufferView, warnOnNonPortableTemplate } = require('internal/fs/utils'); const { @@ -171,11 +168,6 @@ const isWindows = process.platform === 'win32'; const isOSX = process.platform === 'darwin'; -const showStringCoercionDeprecation = deprecate( - () => {}, - 'Implicit coercion of objects with own toString property is deprecated.', - 'DEP0162' -); function showTruncateDeprecation() { if (truncateWarn) { process.emitWarning( @@ -813,7 +805,7 @@ function readvSync(fd, buffers, position) { /** * Writes `buffer` to the specified `fd` (file descriptor). * @param {number} fd - * @param {Buffer | TypedArray | DataView | string | object} buffer + * @param {Buffer | TypedArray | DataView | string} buffer * @param {number | object} [offsetOrOptions] * @param {number} [length] * @param {number | null} [position] @@ -861,9 +853,6 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) { } validateStringAfterArrayBufferView(buffer, 'buffer'); - if (typeof buffer !== 'string') { - showStringCoercionDeprecation(); - } if (typeof position !== 'function') { if (typeof offset === 'function') { @@ -875,7 +864,7 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) { length = 'utf8'; } - const str = String(buffer); + const str = buffer; validateEncoding(str, length); callback = maybeCallback(position); @@ -926,7 +915,7 @@ function writeSync(fd, buffer, offsetOrOptions, length, position) { result = binding.writeBuffer(fd, buffer, offset, length, position, undefined, ctx); } else { - validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer'); + validateStringAfterArrayBufferView(buffer, 'buffer'); validateEncoding(buffer, length); if (offset === undefined) @@ -2154,7 +2143,7 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) { /** * Asynchronously writes data to the file. * @param {string | Buffer | URL | number} path - * @param {string | Buffer | TypedArray | DataView | object} data + * @param {string | Buffer | TypedArray | DataView} data * @param {{ * encoding?: string | null; * mode?: number; @@ -2171,10 +2160,7 @@ function writeFile(path, data, options, callback) { if (!isArrayBufferView(data)) { validateStringAfterArrayBufferView(data, 'data'); - if (typeof data !== 'string') { - showStringCoercionDeprecation(); - } - data = Buffer.from(String(data), options.encoding || 'utf8'); + data = Buffer.from(data, options.encoding || 'utf8'); } if (isFd(path)) { @@ -2201,7 +2187,7 @@ function writeFile(path, data, options, callback) { /** * Synchronously writes data to the file. * @param {string | Buffer | URL | number} path - * @param {string | Buffer | TypedArray | DataView | object} data + * @param {string | Buffer | TypedArray | DataView} data * @param {{ * encoding?: string | null; * mode?: number; @@ -2214,10 +2200,7 @@ function writeFileSync(path, data, options) { if (!isArrayBufferView(data)) { validateStringAfterArrayBufferView(data, 'data'); - if (typeof data !== 'string') { - showStringCoercionDeprecation(); - } - data = Buffer.from(String(data), options.encoding || 'utf8'); + data = Buffer.from(data, options.encoding || 'utf8'); } const flag = options.flag || 'w'; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index f93ecd0cf2f85b..4f9dfb7dd0bd57 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -66,7 +66,7 @@ const { validateOffsetLengthWrite, validateRmOptions, validateRmdirOptions, - validatePrimitiveStringAfterArrayBufferView, + validateStringAfterArrayBufferView, warnOnNonPortableTemplate, } = require('internal/fs/utils'); const { opendir } = require('internal/fs/dir'); @@ -608,7 +608,7 @@ async function write(handle, buffer, offsetOrOptions, length, position) { return { bytesWritten, buffer }; } - validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer'); + validateStringAfterArrayBufferView(buffer, 'buffer'); validateEncoding(buffer, length); const bytesWritten = (await binding.writeString(handle.fd, buffer, offset, length, kUsePromises)) || 0; @@ -838,7 +838,7 @@ async function writeFile(path, data, options) { const flag = options.flag || 'w'; if (!isArrayBufferView(data) && !isCustomIterable(data)) { - validatePrimitiveStringAfterArrayBufferView(data, 'data'); + validateStringAfterArrayBufferView(data, 'data'); data = Buffer.from(data, options.encoding || 'utf8'); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index d1e521426f0bd3..20d5761376d3ee 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -14,7 +14,6 @@ const { MathMin, MathRound, ObjectIs, - ObjectPrototypeHasOwnProperty, ObjectSetPrototypeOf, ReflectApply, ReflectOwnKeys, @@ -874,27 +873,6 @@ const getValidMode = hideStackFrames((mode, type) => { }); const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => { - if (typeof buffer === 'string') { - return; - } - - if ( - typeof buffer === 'object' && - buffer !== null && - typeof buffer.toString === 'function' && - ObjectPrototypeHasOwnProperty(buffer, 'toString') - ) { - return; - } - - throw new ERR_INVALID_ARG_TYPE( - name, - ['string', 'Buffer', 'TypedArray', 'DataView'], - buffer - ); -}); - -const validatePrimitiveStringAfterArrayBufferView = hideStackFrames((buffer, name) => { if (typeof buffer !== 'string') { throw new ERR_INVALID_ARG_TYPE( name, @@ -958,6 +936,5 @@ module.exports = { validateRmOptionsSync, validateRmdirOptions, validateStringAfterArrayBufferView, - validatePrimitiveStringAfterArrayBufferView, warnOnNonPortableTemplate }; diff --git a/test/parallel/test-fs-write-file-sync.js b/test/parallel/test-fs-write-file-sync.js index 950b571c79eb39..53a69a92564f1c 100644 --- a/test/parallel/test-fs-write-file-sync.js +++ b/test/parallel/test-fs-write-file-sync.js @@ -102,20 +102,20 @@ tmpdir.refresh(); assert.strictEqual(content, 'hello world!'); } -// Test writeFileSync with an object with an own toString function +// Test writeFileSync with an invalid input { - // Runtime deprecated by DEP0162 - common.expectWarning('DeprecationWarning', - 'Implicit coercion of objects with own toString property is deprecated.', - 'DEP0162'); - const file = path.join(tmpdir.path, 'testWriteFileSyncStringify.txt'); - const data = { - toString() { - return 'hello world!'; - } - }; - - fs.writeFileSync(file, data, { encoding: 'utf8', flag: 'a' }); - const content = fs.readFileSync(file, { encoding: 'utf8' }); - assert.strictEqual(content, String(data)); + const file = path.join(tmpdir.path, 'testWriteFileSyncInvalid.txt'); + for (const data of [ + false, 5, {}, [], null, undefined, true, 5n, () => {}, Symbol(), new Map(), + new String('notPrimitive'), + { [Symbol.toPrimitive]: (hint) => 'amObject' }, + { toString() { return 'amObject'; } }, + Promise.resolve('amPromise'), + common.mustNotCall(), + ]) { + assert.throws( + () => fs.writeFileSync(file, data, { encoding: 'utf8', flag: 'a' }), + { code: 'ERR_INVALID_ARG_TYPE' } + ); + } } diff --git a/test/parallel/test-fs-write.js b/test/parallel/test-fs-write.js index a321f1b27adaa3..3ed02e25fdcf9e 100644 --- a/test/parallel/test-fs-write.js +++ b/test/parallel/test-fs-write.js @@ -33,7 +33,6 @@ const fn = path.join(tmpdir.path, 'write.txt'); const fn2 = path.join(tmpdir.path, 'write2.txt'); const fn3 = path.join(tmpdir.path, 'write3.txt'); const fn4 = path.join(tmpdir.path, 'write4.txt'); -const fn5 = path.join(tmpdir.path, 'write5.txt'); const expected = 'ümlaut.'; const constants = fs.constants; @@ -127,23 +126,6 @@ fs.open(fn3, 'w', 0o644, common.mustSucceed((fd) => { })); -// Test write with an object with an own toString function -// Runtime deprecated by DEP0162 -common.expectWarning('DeprecationWarning', - 'Implicit coercion of objects with own toString property is deprecated.', - 'DEP0162'); -fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => { - const done = common.mustSucceed((written) => { - assert.strictEqual(written, Buffer.byteLength(expected)); - fs.closeSync(fd); - }); - - const data = { - toString() { return expected; } - }; - fs.write(fd, data, done); -})); - [false, 'test', {}, [], null, undefined].forEach((i) => { assert.throws( () => fs.write(i, common.mustNotCall()), @@ -162,9 +144,12 @@ fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => { }); [ - false, 5, {}, [], null, undefined, + false, 5, {}, [], null, undefined, true, 5n, () => {}, Symbol(), new Map(), new String('notPrimitive'), { [Symbol.toPrimitive]: (hint) => 'amObject' }, + { toString() { return 'amObject'; } }, + Promise.resolve('amPromise'), + common.mustNotCall(), ].forEach((data) => { assert.throws( () => fs.write(1, data, common.mustNotCall()), @@ -184,7 +169,7 @@ fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => { { // Regression test for https://github.com/nodejs/node/issues/38168 - const fd = fs.openSync(fn5, 'w'); + const fd = fs.openSync(fn4, 'w'); assert.throws( () => fs.writeSync(fd, 'abc', 0, 'hex'),