Skip to content
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.

Sign up for GitHub

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

refactor(ext/node): align error messages #25917

Closed
wants to merge 1 commit into from
Closed

refactor(ext/node): align error messages #25917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ext/node/polyfills/_fs/_fs_writeFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export function writeFile(
optOrCallback instanceof Function ? undefined : optOrCallback;

if (!callbackFn) {
throw new TypeError("Callback must be a function.");
throw new TypeError("Callback must be a function");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is nice, but one thing about ext/node is probably the error messages should align with whatever node does for better compability? By doing this are we making the compatiblity worse or are these error messages already not aligned with node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. It seems they are not the same either way:

fs.writeFile('/Users/irbull/test.txt', content, undefined, undefined);

In node (v21.6.2) I get:

TypeError [ERR_INVALID_ARG_TYPE]: The "cb" argument must be of type function. Received undefined

In Deno (1.46.3) I get:

TypeError: Callback must be a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to pump the brakes on the node compat stuff. I don't know if we should consider the error messages part of the API surface. It seems like it might have to be, but then we should align with what Node does. That would be a pretty big undertaking. Have you seen reports of inconsistencies causing problems?

}

pathOrRid = pathOrRid instanceof URL ? pathFromURL(pathOrRid) : pathOrRid;
Expand Down
4 changes: 2 additions & 2 deletions ext/node/polyfills/_util/std_asserts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export function equal(c: unknown, d: unknown): boolean {
!(ObjectPrototypeIsPrototypeOf(WeakMapPrototype, a) &&
ObjectPrototypeIsPrototypeOf(WeakMapPrototype, b))
) return false;
throw new TypeError("cannot compare WeakMap instances");
throw new TypeError("Cannot compare WeakMap instances");
}
if (
ObjectPrototypeIsPrototypeOf(WeakSetPrototype, a) ||
Expand All @@ -130,7 +130,7 @@ export function equal(c: unknown, d: unknown): boolean {
!(ObjectPrototypeIsPrototypeOf(WeakSetPrototype, a) &&
ObjectPrototypeIsPrototypeOf(WeakSetPrototype, b))
) return false;
throw new TypeError("cannot compare WeakSet instances");
throw new TypeError("Cannot compare WeakSet instances");
}
if (seen.get(a) === b) {
return true;
Expand Down
2 changes: 1 addition & 1 deletion ext/node/polyfills/_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ export function validateIntegerRange(

if (value < min || value > max) {
throw new Error(
`${name} must be >= ${min} && <= ${max}. Value was ${value}`,
`${name} must be >= ${min} && <= ${max}: received ${value}`,
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion ext/node/polyfills/internal/crypto/_randomBytes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export const MAX_SIZE = 4294967295;
function generateRandomBytes(size: number) {
if (size > MAX_SIZE) {
throw new RangeError(
`The value of "size" is out of range. It must be >= 0 && <= ${MAX_SIZE}. Received ${size}`,
`The value of "size" is out of range, it must be >= 0 && <= ${MAX_SIZE}: received ${size}`,
);
}

Expand Down
6 changes: 3 additions & 3 deletions ext/node/polyfills/internal/crypto/_randomInt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ export default function randomInt(
!Number.isSafeInteger(min) ||
typeof max === "number" && !Number.isSafeInteger(max)
) {
throw new Error("max or min is not a Safe Number");
throw new Error('"max" or "min" is not a Safe Number');
}

if (max - min > Math.pow(2, 48)) {
throw new RangeError("max - min should be less than 2^48!");
throw new RangeError("max - min must be less than 2^48");
}

if (min >= max) {
throw new Error("Min is bigger than Max!");
throw new Error('"min" is bigger than "max"');
}

min = Math.ceil(min);
Expand Down
4 changes: 2 additions & 2 deletions ext/node/polyfills/internal/crypto/cipher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export class Cipheriv extends Transform implements Cipher {
this.#needsBlockCache =
!(cipher == "aes-128-gcm" || cipher == "aes-256-gcm");
if (this.#context == 0) {
throw new TypeError("Unknown cipher");
throw new TypeError(`Unknown cipher: ${cipher}`);
}
}

Expand Down Expand Up @@ -347,7 +347,7 @@ export class Decipheriv extends Transform implements Cipher {
this.#needsBlockCache =
!(cipher == "aes-128-gcm" || cipher == "aes-256-gcm");
if (this.#context == 0) {
throw new TypeError("Unknown cipher");
throw new TypeError(`Unknown cipher: ${cipher}`);
}
}

Expand Down
2 changes: 1 addition & 1 deletion ext/node/polyfills/internal/crypto/diffiehellman.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,7 @@ export class ECDH {

const c = ellipticCurves.find((x) => x.name == curve);
if (c == undefined) {
throw new Error("invalid curve");
throw new Error(`Invalid curve: ${curve}`);
}

this.#curve = c;
Expand Down
4 changes: 2 additions & 2 deletions ext/node/polyfills/internal/crypto/scrypt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export function scryptSync(
const blen = p * 128 * r;

if (32 * r * (N + 2) * 4 + blen > maxmem) {
throw new Error("exceeds max memory");
throw new Error('"scryptSync" exceeds max memory');
}

const buf = Buffer.alloc(keylen);
Expand Down Expand Up @@ -104,7 +104,7 @@ export function scrypt(

const blen = p * 128 * r;
if (32 * r * (N + 2) * 4 + blen > maxmem) {
throw new Error("exceeds max memory");
throw new Error('"scryptSync" exceeds max memory');
}

try {
Expand Down
8 changes: 6 additions & 2 deletions ext/node/polyfills/internal/crypto/sig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,9 @@ export function signOneShot(
let result: Buffer;
if (op_node_get_asymmetric_key_type(handle) === "ed25519") {
if (algorithm != null && algorithm !== "sha512") {
throw new TypeError("Only 'sha512' is supported for Ed25519 keys");
throw new TypeError(
`Only 'sha512' is supported for Ed25519 keys: received '${algorithm}'`,
);
}
result = new Buffer(64);
op_node_sign_ed25519(handle, data, result);
Expand Down Expand Up @@ -314,7 +316,9 @@ export function verifyOneShot(
let result: boolean;
if (op_node_get_asymmetric_key_type(handle) === "ed25519") {
if (algorithm != null && algorithm !== "sha512") {
throw new TypeError("Only 'sha512' is supported for Ed25519 keys");
throw new TypeError(
`Only 'sha512' is supported for Ed25519 keys: received '${algorithm}'`,
);
}
result = op_node_verify_ed25519(handle, data, signature);
} else if (algorithm == null) {
Expand Down
2 changes: 1 addition & 1 deletion ext/node/polyfills/path/_posix.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function resolve(...pathSegments: string[]): string {
// deno-lint-ignore no-explicit-any
const { Deno } = globalThis as any;
if (typeof Deno?.cwd !== "function") {
throw new TypeError("Resolved a relative path without a CWD.");
throw new TypeError("Resolved a relative path without a CWD");
}
path = Deno.cwd();
}
Expand Down
4 changes: 2 additions & 2 deletions ext/node/polyfills/path/_win32.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,14 @@ export function resolve(...pathSegments: string[]): string {
path = pathSegments[i];
} else if (!resolvedDevice) {
if (typeof Deno?.cwd !== "function") {
throw new TypeError("Resolved a drive-letter-less path without a CWD.");
throw new TypeError("Resolved a drive-letter-less path without a CWD");
}
path = Deno.cwd();
} else {
if (
typeof Deno?.env?.get !== "function" || typeof Deno?.cwd !== "function"
) {
throw new TypeError("Resolved a relative path without a CWD.");
throw new TypeError("Resolved a relative path without a CWD");
}
path = Deno.cwd();

Expand Down
6 changes: 3 additions & 3 deletions ext/node/polyfills/v8.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export function serialize(value: any) {
export function deserialize(buffer: Buffer | ArrayBufferView | DataView) {
if (!isArrayBufferView(buffer)) {
throw new TypeError(
"buffer must be a TypedArray or a DataView",
"Cannot deserialize: 'buffer' must be a 'TypedArray' or a 'DataView'",
);
}
const der = new DefaultDeserializer(buffer);
Expand Down Expand Up @@ -141,7 +141,7 @@ export class Serializer {
writeRawBytes(source: ArrayBufferView): void {
if (!isArrayBufferView(source)) {
throw new TypeError(
"source must be a TypedArray or a DataView",
"Cannot write bytes: 'source' must be a 'TypedArray' or a 'DataView'",
);
}
op_v8_write_raw_bytes(this[kHandle], source);
Expand Down Expand Up @@ -169,7 +169,7 @@ export class Deserializer {
constructor(buffer: ArrayBufferView) {
if (!isArrayBufferView(buffer)) {
throw new TypeError(
"buffer must be a TypedArray or a DataView",
"Cannot construct 'Deserializer': 'buffer' must be a 'TypedArray' or a 'DataView'",
);
}
this.buffer = buffer;
Expand Down
6 changes: 3 additions & 3 deletions tests/node_compat/test/parallel/test-v8-serdes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,15 +265,15 @@ const objects = [
serializer.writeHeader();
assert.throws(
() => serializer.writeRawBytes(INVALID_SOURCE),
/^TypeError: source must be a TypedArray or a DataView$/,
/^TypeError: Cannot write bytes: 'source' must be a 'TypedArray' or a 'DataView'$/,
);
assert.throws(
() => v8.deserialize(INVALID_SOURCE),
/^TypeError: buffer must be a TypedArray or a DataView$/,
/^TypeError: Cannot deserialize: 'buffer' must be a 'TypedArray' or a 'DataView'$/,
);
assert.throws(
() => new v8.Deserializer(INVALID_SOURCE),
/^TypeError: buffer must be a TypedArray or a DataView$/,
/^TypeError: Cannot construct 'Deserializer': 'buffer' must be a 'TypedArray' or a 'DataView'$/,
);
}

Expand Down
2 changes: 1 addition & 1 deletion tests/unit_node/_fs/_fs_writeFile_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ Deno.test("Callback must be a function error", function fn() {
writeFile("some/path", "some data", "utf8");
},
TypeError,
"Callback must be a function.",
"Callback must be a function",
);
});

Expand Down