fix(publish): handle case where multiple config list is present

[kenrick95]

In lib/publish.js, handle case where multiple config list is present. As I observed, this.npm.config.list could contain more than 1 entry if there are multiple .npmrc in the path

When not handled, when there are multiple entries in this.npm.config.list, it causes crash as described in #2834

The change here merge everything in this.npm.config.list, because as I observed through trial and error, the default config is present only at the last entry.

I also added test case that will fail if I don't change lib/publish.js

References

Fixes #2834

[wraithgar]

Thank you so much for tracking this down and fixing it. The test looks great.

I agree with @ljharb's suggestion to flatten through Object.assign.

[wraithgar]

From: https://github.com/npm/config

A list sorted in priority of all the config data objects in the prototype chain. config.list[0] is the cli level, config.list[1] is the env level, and so on.

We need to readdress this it looks like. There are two places where we only look at config.list[0], here and in flatOptions. This appears wrong on its surface. We should either ignore that altogether and only look at the resulting final values that config sets, or parse this consistently between here and flatOptions.

I will be digging into this more today

[wraithgar]

Ok we figured this out. npm.config.list[0] does have all the right consolidated values, but object spread does not work properly on it because they don't all have them as "own properties". The solution is going to be to flatten both items and spread the results of those two. I'll suggest the fix in this PR.

[kenrick95]

@wraithgar Thanks for explaining config.list[0], let me apply your suggestion!

[kenrick95]

ok this seem to work, I need to reverse the config.list first when merging them

[wraithgar]

Ah ok I think I see what happened here. Your tests probably started failing cause their config.list arrays don't reflect what the real config does. In the real config npm.config.list[0] has every value from all the higher-indexed configs. So you only need to worry about the first one in the list. My original suggestion of object spreading the results of flatten on both publishConfig and this.npm.config.list[0] is the right solution I think.

[kenrick95]

hmm the other tests failed too 🤔 https://github.com/npm/cli/runs/2117657010?check_suite_focus=true#step:5:94

[wraithgar]

Ok figured out the test failures. It's because flatten blows up if you pass it just the publishConfig, since it is not a full config item.
This makes the tests pass in latest and I would assume also make them pass in your branch.

return flatten({...flatten(this.npm.config.list[0]), ...publishConfig})

[wraithgar]

It's goofy that we have to double-flatten but that's the shortest step between here and any long term fix we implement.

[kenrick95]

I see I see, I've committed this but found out that the return value of flatten isn't that compatible as an argument for another flatten

This test case of mine failed:

const tag = 'better-tag'

const configList = [
    { ...defaults, tag },
    { ...defaults, registry: `https://other.registry`, tag: 'some-tag' },
    defaults,
  ]

const Publish = requireInject('../../lib/publish.js', {
    libnpmpublish: {
      publish: (manifest, tarData, opts) => {
        t.same(opts.defaultTag, tag, 'gets option for expected tag') // defaultTag here is undefined!?
      },
    },
  })

This is because flatten(this.npm.config.list[0]) takes in an object that has tag field and will return an object that has a field named defaultTag; and on the second flatten call, this field will never be read, i.e. flatten(flatten(this.npm.config.list[0])), which causes defaultTag field to be undefined

[kenrick95]

I also added this assertion to check whether we're applying the config in the correct order

[wraithgar]

@kenrick95 Thanks again for finding the root cause of this bug and working on this PR. There is another PR in-flight right now that is going to supersede the code at fault here, but we'll be sure to get this PR into the release before that so at least the fix is represented. We won't worry so much about the CI passing here, as the other PR will take care of that (or we'll work it out when we get the two merged into the same branch, instead of having to try to do it twice.

The other PR is here #2878

[andria-dev]

Any word on when this fix will be published?

[wraithgar]

Any word on when this fix will be published?

This week