Version 5.2.0+ -- TypeError: Cannot read property 'reduce' of undefined

[jamonholmgren]

When upgrading gluegun from Cosmiconfig 5.1.0 to 5.2.0, I'm getting an error in my tests:

  ● runs a command explicitly

    TypeError: Cannot read property 'reduce' of undefined

      at getPropertyByPath (node_modules/cosmiconfig/dist/getPropertyByPath.js:15:21)
      at Explorer.loadPackageProp (node_modules/cosmiconfig/dist/createExplorer.js:177:30)
      at Explorer.loadFileContent (node_modules/cosmiconfig/dist/createExplorer.js:230:12)
      at Explorer.createCosmiconfigResultSync (node_modules/cosmiconfig/dist/createExplorer.js:263:31)
      at Explorer.loadSearchPlaceSync (node_modules/cosmiconfig/dist/createExplorer.js:158:17)
      at Explorer.searchDirectorySync (node_modules/cosmiconfig/dist/createExplorer.js:136:21)
      at run (node_modules/cosmiconfig/dist/createExplorer.js:108:27)
      at cacheWrapper (node_modules/cosmiconfig/dist/cacheWrapper.js:14:18)
      at Explorer.searchFromDirectorySync (node_modules/cosmiconfig/dist/createExplorer.js:117:14)
      at Explorer.searchSync (node_modules/cosmiconfig/dist/createExplorer.js:102:17)

  ● runs a command via passed in args

    TypeError: Cannot read property 'reduce' of undefined

      at getPropertyByPath (node_modules/cosmiconfig/dist/getPropertyByPath.js:15:21)
      at Explorer.loadPackageProp (node_modules/cosmiconfig/dist/createExplorer.js:177:30)
      at Explorer.loadFileContent (node_modules/cosmiconfig/dist/createExplorer.js:230:12)
      at Explorer.createCosmiconfigResultSync (node_modules/cosmiconfig/dist/createExplorer.js:263:31)
      at Explorer.loadSearchPlaceSync (node_modules/cosmiconfig/dist/createExplorer.js:158:17)
      at Explorer.searchDirectorySync (node_modules/cosmiconfig/dist/createExplorer.js:136:21)
      at run (node_modules/cosmiconfig/dist/createExplorer.js:108:27)
      at cacheWrapper (node_modules/cosmiconfig/dist/cacheWrapper.js:14:18)
      at Explorer.searchFromDirectorySync (node_modules/cosmiconfig/dist/createExplorer.js:117:14)
      at Explorer.searchSync (node_modules/cosmiconfig/dist/createExplorer.js:102:17)

There are no other changes -- tests pass on 5.1.0 and fail with this error on 5.2.0.

Here's the diff 5.1.0 to 5.2.0: https://github.com/davidtheclark/cosmiconfig/compare/71da3267ff24b3e80bd0e5fc955b4c45942c3548..f1750cde0b9c53bc577efa60d012a85a4dc4b823

If you'd like to experience it for yourself, do this:

git clone https://github.com/infinitered/gluegun.git
cd gluegun
git checkout deps/upgrade-several
yarn test # tests will pass
yarn add cosmiconfig@5.2.0
yarn test # tests will fail

This also happens on the latest cosmiconfig, 5.2.1.

[jamonholmgren]

Submitted PR #189 which will fix this issue.

[davidtheclark]

Thanks @jamonholmgren for the issue and PR. Can you please explain more the specific usage that raises this error? That will help clarify the situation, and will help make a useful changelog entry for the fix.

[jamonholmgren]

Hey @davidtheclark, I'm not entirely sure, to be honest. I only upgraded from cosmiconfig 5.1.0 to 5.2.0 without any other changes to my code and it blew up.

I could try to track down the specific code in Gluegun that is passing in an undefined path, I suppose, but regardless I wouldn't expect behavior to change in a minor release.

Beyond that, the API difference between the hand-rolled version of getPropertyByPath vs _.get was (I assume) unintentional, so aligning those seems prudent. If you don't want that, then I'd assume it should be a major release.

Thanks for taking a look!

[olsonpm]

So the first failing test is relying on cosmiconfig to fail quietly during a unit test where the unit doesn't declare all the data it would otherwise have in a happy path scenario. This is unfortunately caused because we don't enforce the required property of moduleName

e.g.

// the docs say `moduleName` is required, but this succeeds
const explorer = cosmiconfig();

Gluegun is then relying on this behavior and expects the following to return null

const result = cosmiconfig(undefined).searchSync(__dirname)

console.log(result === null)
// v5.1.0 => 'true'
// v5.2.0 => Error

So although I do think this fix should be introduced as a patch in order to make the behavior consistent with previous versions - I could also see wanting to add a check to make sure moduleName is a truthy string.

[davidtheclark]

Thanks for digging in, @olsonpm. So this is one of those pesky cases where usage contrary to the publicly documented API (moduleName is documented as required) had an effect that somebody relied on, and that effect has changed.

I do think this fix should be introduced as a patch in order to make the behavior consistent with previous versions - I could also see wanting to add a check to make sure moduleName is a truthy string.

I would favor fixing the bug (unpredictable behavior when you don't pass a required parameter) rather than codifying the bug as a feature. Since this fix would be perfectly compatible with the publicly documented API, a patch release is what semver expects ("Bug fixes not affecting the API increment the patch version"). (Yes, this might break somebody's build if they're relying on the undocumented buggy behavior, but that's always a danger when fixing a bug.)

If we are going to add type assertions against the moduleName parameter, we may as well also add them against all the options, to prevent similar issues in the future.

Any objections to that proposal: fixing this with parameter type assertions, and releasing that as a patch?

[olsonpm]

No objections here. I just do think this is a value judgement because if we were talking about a larger product whose api has been stable for a long time, then I believe the bug fix similar to this one ought to be considered a breaking change because of the potential negative affect of changing it. In our case I think it's fine though. If enough people run into issues because they depended on similar behavior, we can revert and add the check in a branch for v6.

I can submit a PR to Gluegun to add an if statement to prevent the tests from failing.

[jamonholmgren]

No objections -- you're correct, and I really appreciate the offer of a PR to fix the failing test! Top notch open source support. 💜

[davidtheclark]

Closing as stale. I hope the issue was fixed by the Gluegun PR mentioned above.

[jamonholmgren]

@davidtheclark Thanks, I did manage to track down the undefined issue and was able to upgrade to the latest cosmiconfig. Here's the commit if you're interested:

infinitered/gluegun@c017f06