Node: require.resolve.length should be 1, not 2

[EvgenyOrekhov]

• Version: since 8.9.0
• Platform: any
• Subsystem: module

According to the docs, require.resolve() has the following signature:

require.resolve(request[, options])

which means the options parameter is optional, but require.resolve.length === 2, which is wrong, it should be 1.

Example:

// A function with an optional parameter:
const f = (a, b = undefined) => console.log(a, b);
f.length; // evaluates to 1

require.resolve.length; // evaluates to 2, which is wrong

The exact line of this bug: https://github.com/nodejs/node/blob/841e305e4c0bc9f633006a293b893fd69dbbf596/lib/internal/module.js#L17

options here should either be declared using ES6 default parameter syntax:

function resolve(request, options = undefined) {

or require.resolve.length should be explicitly set to 1 using Object.defineProperty():

Object.defineProperty(
    require.resolve,
    "length",
    {value: 1}
);

[mscdex]

I wouldn't consider this a bug. Just because there is an explicit parameter does not mean a value for that parameter is required. This is the case throughout node core, not just require.resolve().

Also, explicitly setting/overriding the .length property of the function is unnecessary. Besides, what happens when someone monkey-patches that function? The length may no longer be what you expect.

[EvgenyOrekhov]

@mscdex Here is a quote from ECMAScript 2018 Language Specification:

Optional parameters (which are indicated with brackets: [ ]) or rest parameters (which are shown using the form «...name») are not included in the default argument count.

I think Node.js core modules should behave consistently with ECMAScript standard built-in modules.

[joyeecheung]

@EvgenyOrekhov ECMAScript built-in modules follow the spec while Node.js modules (are supposed to) follow the Node.js API documentation, but should the the notation in the Node.js API documentation be consistent with the ECMAScript spec? Personally I don't think so, because spec and documentations are different things. The optional parameter syntax in the Node.js documentation just happens to be similar to the one in the spec, does not mean it's intended to behave the same.

[cjihrig]

Node also has a number of functions that I don't think this would be really practical for. https://nodejs.org/api/net.html#net_server_listen, for example.

[EvgenyOrekhov]

@joyeecheung It's not about the notation, it's about required and optional parameters and about the argument count. If optional parameters are not included in the default argument count in ECMAScript standard built-in modules, I expect Node.js core module to behave the same way. More than that, as a JavaScript developer, I expect every JavaScript library to behave the same way ECMAScript standard built-in modules do.

[addaleax]

I would consider this a non-bug as well. Because arguments and optional parameters exist, and because functions can choose to ignore parameters, you can’t rely on fn.length for anything in JS, really.

Or to put it another way: Whether an argument is optional is a question of the function’s semantics, not of the information a function exposes as its properties in JS, so either way the docs are the source of truth here, not fn.length.

[EvgenyOrekhov]

@addaleax Whether it's a bug or not, but the fact remains that I used require.resolve() with Ramda, and it worked before Node.js 8.9.0, but it broke in Node.js 8.9.0 because Ramda's curry() depends on fn.length.

[EvgenyOrekhov]

@addaleax And therefore every new optional parameter in any function that is changing function's length is a breaking change. It is a wrong way to extend software systems.

[mscdex]

IMO depending on fn.length is wrong unless you're doing it with your own code that you control.

[bnoordhuis]

I'll close this out. It's clear there is no consensus it's a bug, rather the opposite.