AggregateError argument order seems backwards

[jridgewell]

Why is errors the first parameter? Shouldn't message be first?

If someone were to switch from a TypeError to an AggregateError, it'd be surprising that message is now second.

As a bonus, if there are multiple errors, it's pleasing that the array can be multiple lines without a trailing message argument. Kinda like how node's callback is always last:

new AggregateError('message', [
  error1,
  error2,
  error3,
  error4,
]);

[ljharb]

This was discussed in #14.

[jridgewell]

I think "optional arguments follow mandatory arguments" is a stronger principle than "subclasses should have the same signature as their superclass", especially given the existence of additional optional arguments on all Error subclasses in many implementations, so I am inclined to put the errors parameter before message. — https://github.com/tc39/proposal-promise-any/issues/14#issuecomment-468094826

Let's make errors optional? Doesn't hurt much, but it'd let us put the arguments in the correct (least surprising) order.

[bakkot]

That was also discussed (and rejected, at least by e.g. me) in #14.

Fwiw I would also find it more surprising to have message first, precisely because it seems optional and errors does not.

[jridgewell]

precisely because it seems optional and errors does not.

Neither seems optional. But if we have to have "optional" things first, then making them both optional lets us use the least surprising ordering.

[bakkot]

Neither seems optional.

I don't share this intuition at all. I encounter (new Error()).stack or throw new RangeError(), etc, fairly frequently.

lets us use the least surprising ordering

Again, I would find your proposed ordering more surprising. But also, even if this were not the case, all of

• errors being non-optional (as discussed in #14),
• message being optional, as with every other error type, and
• optional arguments following non-optional ones

seem like more important principles, and collectively imply the current ordering.

Incidentally, the current ordering also matches other error types (OverconstrainedError, RTCError) in the web platform. (The latter of those at my request, but it's shipping now.)

[chicoxyzzy]

Currently it's not possible to switch order of arguments for AggregateError because of current design of AggregateError, so I'm going to close this issue. We can reopen it if and when we'll realize to make errors an optional argument.

[jridgewell]

I still think this is ridiculous. If we want errors to be non-optional, allow it to be the first or second param. It's not uncommon for functions to juggle their parameters, and we're even discussing it in TC39 with Number.range(to).

class AggregateError {
  constructor(messageOrErrors, errors) {
    if (typeof messageOrErrors !== 'string') {
      errors = messageOrErrors;
      messageOrErrors = '';
    }

    // ...
  }
}

[ljharb]

While i think the arg order should be errors, message, i think functions “juggling” their params is a massive antipattern, and other than substring i don’t know of any examples in 262. In any proposal, I’d be staunchly opposed to doing that.

[jridgewell]

i think functions “juggling” their params is a massive antipattern

I think you're using it much more than you realize. Just doing a doc search, I can find:

Node:

- `assert.doesNotReject(asyncFn[, error][, message])` - `assert.doesNotThrow(fn[, error][, message])` - `assert.rejects(asyncFn[, error][, message])` - `assert.throws(fn[, error][, message])` - `buf.fill(value[, offset[, end]][, encoding])` - `buf.includes(value[, byteOffset][, encoding])` - `buf.indexOf(value[, byteOffset][, encoding])` - `buf.lastIndexOf(value[, byteOffset][, encoding])` - `buf.write(string[, offset[, length]][, encoding])` - `child_process.exec(command[, options][, callback])` - `child_process.execFile(file[, args][, options][, callback])` - `child_process.execFileSync(file[, args][, options])` - `child_process.fork(modulePath[, args][, options])` - `child_process.spawn(command[, args][, options])` - `child_process.spawnSync(command[, args][, options])` - `cipher.update(data[, inputEncoding][, outputEncoding])` - `console.error([data][, ...args])` - `console.info([data][, ...args])` - `console.log([data][, ...args])` - `console.timeLog([label][, ...data])` - `console.trace([message][, ...args])` - `console.warn([data][, ...args])` - `crypto.createDiffieHellman(prime[, primeEncoding][, generator][, generatorEncoding])` - `crypto.randomFill(buffer[, offset][, size], callback)` - `crypto.randomFillSync(buffer[, offset][, size])` - `crypto.scrypt(password, salt, keylen[, options], callback)` - `decipher.update(data[, inputEncoding][, outputEncoding])` - `diffieHellman.computeSecret(otherPublicKey[, inputEncoding][, outputEncoding])` - `dns.lookup(hostname[, options], callback)` - `dns.resolve(hostname[, rrtype], callback)` - `dns.resolve4(hostname[, options], callback)` - `dns.resolve6(hostname[, options], callback)` - `ecdh.computeSecret(otherPublicKey[, inputEncoding][, outputEncoding])` - `ecdh.getPublicKey([encoding][, format])` - `fs.access(path[, mode], callback)` - `fs.appendFile(path, data[, options], callback)` - `fs.copyFile(src, dest[, mode], callback)` - `fs.fstat(fd[, options], callback)` - `fs.ftruncate(fd[, len], callback)` - `fs.lstat(path[, options], callback)` - `fs.mkdir(path[, options], callback)` - `fs.mkdtemp(prefix[, options], callback)` - `fs.open(path[, flags[, mode]], callback)` - `fs.opendir(path[, options], callback)` - `fs.readFile(path[, options], callback)` - `fs.readdir(path[, options], callback)` - `fs.readlink(path[, options], callback)` - `fs.realpath(path[, options], callback)` - `fs.realpath.native(path[, options], callback)` - `fs.rmdir(path[, options], callback)` - `fs.stat(path[, options], callback)` - `fs.symlink(target, path[, type], callback)` - `fs.truncate(path[, len], callback)` - `fs.watch(filename[, options][, listener])` - `fs.watchFile(filename[, options], listener)` - `fs.write(fd, buffer[, offset[, length[, position]]], callback)` - `fs.write(fd, string[, position[, encoding]], callback)` - `fs.writeFile(file, data[, options], callback)` - `fs.writev(fd, buffers[, position], callback)` - `http.createServer([options][, requestListener])` - `http.get(url[, options][, callback])` - `http.request(url[, options][, callback])` - `http2.connect(authority[, options][, listener])` - `http2session.destroy([error][, code])` - `http2session.settings([settings][, callback])` - `http2stream.pushStream(headers[, options], callback)` - `https.createServer([options][, requestListener])` - `https.get(url[, options][, callback])` - `https.request(url[, options][, callback])` - `net.connect(port[, host][, connectListener])` - `net.createConnection(port[, host][, connectListener])` - `net.createServer([options][, connectionListener])` - `new Console(stdout[, stderr][, ignoreErrors])` - `new net.Server([options][, connectionListener])` - `process.emitWarning(warning[, type[, code]][, ctor])` - `process.report.writeReport([filename][, err])` - `process.send(message[, sendHandle[, options]][, callback])` - `readline.cursorTo(stream, x[, y][, callback])` - `request.end([data[, encoding]][, callback])` - `request.setSocketKeepAlive([enable][, initialDelay])` - `request.write(chunk[, encoding][, callback])` - `response.end([data[, encoding]][, callback])` - `response.write(chunk[, encoding][, callback])` - `response.writeHead(statusCode[, statusMessage][, headers])` - `server.listen([port[, host[, backlog]]][, callback])` - `server.listen(handle[, backlog][, callback])` - `server.listen(path[, backlog][, callback])` - `server.setTimeout([msecs][, callback])` - `session.post(method[, params][, callback])` - `socket.bind([port][, address][, callback])` - `socket.connect(port[, address][, callback])` - `socket.connect(port[, host][, connectListener])` - `socket.end([data[, encoding]][, callback])` - `socket.send(msg[, offset, length][, port][, address][, callback])` - `socket.setKeepAlive([enable][, initialDelay])` - `socket.write(data[, encoding][, callback])` - `stream.finished(stream[, options], callback)` - `subprocess.send(message[, sendHandle[, options]][, callback])` - `tls.connect(path[, options][, callback])` - `tls.connect(port[, host][, options][, callback])` - `tls.createSecurePair([context][, isServer][, requestCert][, rejectUnauthorized][, options])` - `tls.createServer([options][, secureConnectionListener])` - `worker.send(message[, sendHandle[, options]][, callback])` - `writable.end([chunk[, encoding]][, callback])` - `writable.write(chunk[, encoding][, callback])` - `writeStream.cursorTo(x[, y][, callback])` - `writeStream.hasColors([count][, env])` - `zlib.brotliCompress(buffer[, options], callback)` - `zlib.brotliDecompress(buffer[, options], callback)` - `zlib.deflate(buffer[, options], callback)` - `zlib.deflateRaw(buffer[, options], callback)` - `zlib.gunzip(buffer[, options], callback)` - `zlib.gzip(buffer[, options], callback)` - `zlib.inflate(buffer[, options], callback)` - `zlib.inflateRaw(buffer[, options], callback)` - `zlib.unzip(buffer[, options], callback)`

Express:

- `app.listen([port[, host[, backlog]]][, callback])` - `app.param([name], callback)` - `app.render(view, [locals], callback)` - `app.use([path,] callback [, callback...])` - `res.download(path [, filename] [, options] [, fn])` - `res.end([data] [, encoding])` - `res.render(view [, locals] [, callback])` - `res.sendFile(path [, options] [, fn])` - `router.use([path], [function, ...] function)`

For node, this is extremely common, because the overriding pattern is "callback goes last". If you want optional parameters, they go before that callback. I think this can be an excellent API pattern, and anything else would have felt worse.

(I can find more, but they're the easiest ones to find are always Node ones because they follow this pattern).

other than substring i don’t know of any examples in 262

I don't think we've had the need in 262 yet, so there aren't any. I can't really think of any API that would have been made better by this, until now.

In any proposal, I’d be staunchly opposed to doing that.

As in https://github.com/tc39/proposal-Number.range/issues/18, I think this an extremely reasonable pattern to use.

[ljharb]

node’s and express’ patterns were born and largely locked in by 2012, long before that became a widely accepted antipattern. It’s because i use it a lot that i understand why it’s suboptimal.

[gibson042]

I don't think we've had the need in 262 yet, so there aren't any. I can't really think of any API that would have been made better by this, until now.

Not that it justifies anything here, but the Function constructors interpret their arguments as a list of parameter names followed by a body (and the Array constructor arguably also juggles for Array(len) vs. Array(...items)).