Return undefined as pkg for core modules

[mpal9000]

Hello, thanks for everything you share! This pull request is about a small issue that I have when combining "resolve" with "thunkify" and generators.

var resolve = thunkify(require('resolve'));

function* gen() {
  // I want to write
  let [filePath, pkg] = yield resolve(modulePath, opts);

  // But when the `modulePath` is a core module's name (eg. "path"), I have to write
  let res = yield resolve(modulePath, opts);
  let filePath = Array.isArray(res) ? res[0] : res;
  let pkg = Array.isArray(res) ? res[1] : null;
}

[ljharb]

this is switching it from calling with undefined to calling with null - I'm not sure why this is an improvement?

[mpal9000]

const co = require('co')
const thunkify = require('thunkify')
const resolve = thunkify(require('resolve'))

function* gen(modulePath) {
  const [filePath, pkg] = yield resolve(modulePath, {})
  return [filePath, pkg]
}

co(function *(){
  console.log('resolved:', yield gen('path'))
}).catch((err) =>{
  console.error(err.stack);
})

// unpatched:
// => resolved: [ 'p', 'a' ]

// patched:
// => resolved: [ 'path', null ]

[ljharb]

ok so I'm trying to unpack that code sample - it seems like gen could simply be function* gen(modulePath) { return (yield resolve(modulePath, {})).slice(0, 2); } so if the callback is called with x, null versus called with x, undefined, those should be equivalent. I guess I'm not seeing where the 'a' is coming from when unpatched.

[mpal9000]

Hello, the problem is a rare case of libraries combination.
When multiple values are returned from a thunkified function that is run inside "co", the result is an array. See https://github.com/tj/co/blob/master/index.js#L139
Maybe it should always return an array, but it's about convenience vs. consistency.
On the other side "resolve", as a lower level lib, could handle problems like this, by always returning the same number of values.

Also your "slice(0, 2)" version inside the generator function, is not equivalent for the "unpatched" version. The first one does array destructuring, so it expects the sourced result to be an array and returns an array. The second one has the same behavior only for an array result, but for single value results it returns a string.

const [filePath, pkg] = yield resolve('path')
return [filePath, pkg] //=> ['p', 'a']
// vs.
return (yield resolve('path')).slice(0, 2) //=> 'pa'

In the "patched" version we get [ 'path', null ] from both.

[ljharb]

Hmm - I'm not super comfortable about working around what is definitely a bug - for convenience or no - in co.

This also is a breaking change for anyone who's currently testing === undefined or similar.

[mpal9000]

What about now :-)

[mpal9000]

we get [ 'path', undefined ] with the explicit undefined pkg argument

[ljharb]

I know that I'm being a bit pedantic, but while this is better, now someone relying on arguments.length being 2 to signal an error case will be broken.

I really think this is a bug that needs fixing in co, otherwise you'll be making PRs all over the ecosystem to pass explicit undefined values everywhere.

[mpal9000]

No problem, but there is no error case here to catch with arguments.length.

[mpal9000]

who does this? you always write if (err) throw ...

[ljharb]

If it's possible to observe the breaking change, then it's a breaking change, even if nobody really does it.

You might use arguments.length to determine if you should require the 3rd arg or not, for example. It's more explicit than checking for undefined or falsiness.

[mpal9000]

I think that in the case of the 3rd "package" argument, either you are interested to read it or not and you want to get it in a convenient way. I cannot disagree about the extremely rare breaking usage though.

[olsonpm]

fyi mpal9000 - I use arguments.length to validate input all the time. Like ljharb mentions, it guarantees whether the developer passed an argument as opposed to checking whether they accidentally passed in an undefined variable. Mostly this distinction helps with friendly error messages such as "method requires exactly two arguments" vs "the second argument must be a sting".

In fact, that distinction is one of the reasons I may choose to use function over () =>

[ljharb]

I'm going to close this; passing nothing is strictly better than passing undefined.

This is a flaw in co, and it should be fixed there.