Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

fix: pass the scope of the package to pacote #158

Closed
wants to merge 3 commits into from
Closed

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Oct 14, 2020

this allows npm-registry-fetch to correctly identify the registry associated with the request, which resolves the issues with incorrect auth being sent

References

npm/cli#1959
npm/cli#1960
npm/cli#1800

this allows npm-registry-fetch to correctly identify
the registry associated with the request, which resolves
the issues with incorrect auth being sent
@nlf nlf requested a review from isaacs October 14, 2020 16:05
@isaacs
Copy link
Contributor

isaacs commented Oct 14, 2020

🤦

Yes, lgtm.

@isaacs
Copy link
Contributor

isaacs commented Oct 14, 2020

Just a thought, could we also fix this by changing

const res = node.resolved ? this[_registryResolved](node.resolved)

to

const res = node.resolved ? `${node.name}@${this[_registryResolved](node.resolved)}`

@nlf
Copy link
Contributor Author

nlf commented Oct 14, 2020

Yup, that also works since it allows npm-package-args to pick up the scope instead

@isaacs
Copy link
Contributor

isaacs commented Oct 14, 2020

Ok. Might be safer since it won't clobber a --scope config being passed in explicitly? I'm not sure offhand how that would affect this scenario.

Instead of manually setting a scope property that gets forwarded to
pacote.extract, uses a spec that sets the resolved value so that pacote
uses that instead.

Addresses code review from #158
@ruyadorno ruyadorno force-pushed the nlf/fix-scoped-auth branch from bfd3208 to d5068c8 Compare October 14, 2020 22:35
@ruyadorno
Copy link
Contributor

ruyadorno commented Oct 14, 2020

I addressed the CR and added a unit test to assert its expectation but I could not really reproduce the initial issues in a complete e2e test in my local machine, therefore I'm not a 100% sure this fixes the bugs as expected. Let me know if you have a repro around and can test it @nlf

[edit] I ran a bunch of installs + cli tests on it and it doesn't look like this is going to break anything so I'm feeling more confident about landing it 😊

@nlf
Copy link
Contributor Author

nlf commented Oct 15, 2020

tested it against my reproduction and it does fix it 👍

i can't approve a PR i submitted apparently, but it's safe to consider this my approval

@isaacs isaacs closed this in ac51ae8 Oct 15, 2020
@wraithgar wraithgar deleted the nlf/fix-scoped-auth branch April 22, 2021 17:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants