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

feat: add --replace-registry-host=<npmjs|always|never> #4860

Merged
merged 1 commit into from Aug 2, 2022

Conversation

fritzy
Copy link
Contributor

@fritzy fritzy commented May 4, 2022

Dist references in a lockfile currently have their host replaced with the configured registry when it is https://registry.npmjs.org, but not otherwise. Between this PR and a pacote PR, we added the options or never replacing the host or always replacing the host, in addition to the current behavior of npmjs.

@fritzy fritzy requested a review from a team as a code owner May 4, 2022 23:52
@npm-robot
Copy link
Contributor

npm-robot commented May 5, 2022

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 46.239 ±0.27 27.311 ±0.06 15.097 ±0.08 17.688 ±0.80 2.484 ±0.06 2.415 ±0.02 1.937 ±0.06 10.058 ±0.05 1.886 ±0.04 2.912 ±0.05
#4860 47.964 ±2.48 33.293 ±8.26 31.379 ±21.79 18.308 ±1.20 2.519 ±0.01 2.522 ±0.00 1.990 ±0.00 10.152 ±0.03 1.979 ±0.00 3.040 ±0.22
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 43.285 ±14.62 20.209 ±0.02 10.852 ±0.00 11.769 ±0.10 2.289 ±0.03 2.309 ±0.02 2.055 ±0.01 7.531 ±0.00 1.897 ±0.02 2.637 ±0.04
#4860 34.733 ±1.57 21.350 ±0.14 11.447 ±0.04 12.078 ±0.18 2.295 ±0.02 2.289 ±0.00 2.062 ±0.07 7.419 ±0.12 1.883 ±0.00 2.632 ±0.06

test/lib/npm.js Outdated Show resolved Hide resolved
@fritzy fritzy force-pushed the fritzy/registry-host branch 2 times, most recently from 0bbdecb to 0bd6d2f Compare May 5, 2022 17:05
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this looks good, but i did see one thing that may cause an issue for some folks

workspaces/arborist/lib/arborist/reify.js Outdated Show resolved Hide resolved
@everett1992
Copy link

--replace-registry-host=always will have confusing behavior when part of the pathname belongs to the registry e.g.

# package lock created with registry=https://registries.biz/customer1
resolved=https://registries.biz/customer1/pkg/-/pkg-1.0.0.tgz

# package lock read with registry=https://registries.biz/customer2 replace-registry-host=always
resolved=https://registries.biz/customer2/customer1/pkg/-/pkg-1.0.0.tgz
                                          ^-------- Oh no!

Please consider the record-default-registry option from #4264 which tries to address a similar issue as this PR.

@darcyclarke
Copy link
Contributor

@fritzy what's the state of this PR?

@fritzy
Copy link
Contributor Author

fritzy commented Jul 26, 2022

--replace-registry-host can also be a bare host name like registry.npmjs.org.

@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Jul 26, 2022

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 39.439 ±6.25 18.838 ±0.19 16.472 ±0.07 19.606 ±1.15 2.829 ±0.02 2.832 ±0.03 2.380 ±0.02 11.304 ±0.00 2.264 ±0.00 3.297 ±0.02
#4860 38.232 ±2.37 19.386 ±0.06 16.879 ±0.10 19.748 ±0.63 3.007 ±0.11 2.835 ±0.04 2.285 ±0.00 11.483 ±0.04 2.283 ±0.02 3.436 ±0.19
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 27.032 ±0.88 14.100 ±0.05 12.463 ±0.04 13.884 ±0.21 2.591 ±0.01 2.581 ±0.00 2.271 ±0.00 8.183 ±0.02 2.186 ±0.00 2.962 ±0.00
#4860 26.586 ±0.22 14.287 ±0.06 12.511 ±0.02 13.423 ±0.21 2.626 ±0.01 2.661 ±0.03 2.293 ±0.02 8.337 ±0.00 2.194 ±0.00 3.229 ±0.39

@darcyclarke darcyclarke added the semver:minor new backwards-compatible feature label Jul 26, 2022
@fritzy fritzy requested review from nlf and wraithgar July 27, 2022 17:24
@darcyclarke
Copy link
Contributor

@fritzy did we investigate @everett1992's question & PR? (ie. #4264) Just want to make sure all concerns here are resolved before we add this support

@fritzy
Copy link
Contributor Author

fritzy commented Aug 2, 2022

@darcyclarke yeah, this was broken up into a previous PR with his work in it. This is a separate piece.

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

ok one last thing that needs to be tweaked then i can land this!

workspaces/arborist/lib/arborist/reify.js Show resolved Hide resolved
@nlf nlf merged commit 703dbbf into latest Aug 2, 2022
@nlf nlf deleted the fritzy/registry-host branch August 2, 2022 21:45
@nlf nlf mentioned this pull request Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants