Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

kenrick95
Copy link
Contributor

@kenrick95 kenrick95 commented Mar 15, 2021

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

@kenrick95 kenrick95 requested a review from a team as a code owner March 15, 2021 08:14
lib/publish.js Outdated Show resolved Hide resolved
@wraithgar wraithgar added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release semver:patch semver patch level for changes labels Mar 15, 2021
@wraithgar
Copy link
Member

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.

test/lib/publish.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

wraithgar commented Mar 15, 2021

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
Copy link
Member

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.

lib/publish.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@kenrick95 kenrick95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/lib/publish.js Outdated Show resolved Hide resolved
lib/publish.js Outdated
{},
...[...this.npm.config.list].reverse(),
publishConfig
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const Publish = requireInject('../../lib/publish.js', {
libnpmpublish: {
publish: (manifest, tarData, opts) => {
t.same(opts.defaultTag, tag, 'gets option for expected tag')
Copy link
Contributor Author

@kenrick95 kenrick95 Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wraithgar
Copy link
Member

@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

isaacs pushed a commit that referenced this pull request Mar 18, 2021
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, the default config is present only at the last entry.

Fix: #2834
Co-authored-by: @wraithgar

PR-URL: #2865
Credit: @kenrick95
Close: #2865
Reviewed-by: @isaacs, @wraithgar
isaacs pushed a commit that referenced this pull request Mar 18, 2021
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, the default config is present only at the last entry.

Fix: #2834
Co-authored-by: @wraithgar

PR-URL: #2865
Credit: @kenrick95
Close: #2865
Reviewed-by: @isaacs, @wraithgar
@isaacs isaacs changed the base branch from latest to isaacs/config-refactor March 18, 2021 18:56
@isaacs isaacs closed this Mar 18, 2021
@isaacs isaacs force-pushed the kenrick/multiple-publish-configs branch from cb8a7fa to e04f12f Compare March 18, 2021 18:56
isaacs pushed a commit that referenced this pull request Mar 18, 2021
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, the default config is present only at the last entry.

Fix: #2834
Co-authored-by: @wraithgar

PR-URL: #2865
Credit: @kenrick95
Close: #2865
Reviewed-by: @isaacs, @wraithgar
@kenrick95 kenrick95 deleted the kenrick/multiple-publish-configs branch March 19, 2021 02:54
@andria-dev
Copy link

Any word on when this fix will be published?

@wraithgar
Copy link
Member

Any word on when this fix will be published?

This week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] unable to publish when using publishConfig option - ERR_INVALID_ARG_TYPE
5 participants