Skip to content

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

Merged
nlf merged 1 commit intolatestfrom
fritzy/registry-host
Aug 2, 2022
Merged

feat: add --replace-registry-host=<npmjs|always|never>#4860
nlf merged 1 commit intolatestfrom
fritzy/registry-host

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 •
edited
Loading

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

wraithgar
wraithgar reviewed May 5, 2022
View reviewed changes
@fritzy fritzy force-pushed the fritzy/registry-host branch 2 times, most recently from 0bbdecb to 0bd6d2f Compare May 5, 2022 17:05
nlf
nlf suggested changes May 9, 2022
View reviewed changes
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

@fritzy fritzy force-pushed the fritzy/registry-host branch from 0bd6d2f to 6eb06ed Compare May 9, 2022 23:50
@everett1992
Copy link

everett1992 commented May 10, 2022

--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.

ljharb
ljharb reviewed May 10, 2022
View reviewed changes
@darcyclarke
Copy link
Contributor

darcyclarke commented Jul 26, 2022

@fritzy what's the state of this PR?

@fritzy fritzy force-pushed the fritzy/registry-host branch from 6eb06ed to 2868388 Compare July 26, 2022 20:37
@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 •
edited
Loading

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

darcyclarke commented Aug 1, 2022

@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.

nlf
nlf suggested changes Aug 2, 2022
View reviewed changes
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!

@fritzy fritzy force-pushed the fritzy/registry-host branch from 2868388 to 17721c6 Compare August 2, 2022 21:10
@fritzy fritzy force-pushed the fritzy/registry-host branch from 17721c6 to 2e5dfb9 Compare August 2, 2022 21:21
@nlf nlf mentioned this pull request Aug 2, 2022
Closed
@nlf nlf merged commit 703dbbf into latest Aug 2, 2022
@nlf nlf deleted the fritzy/registry-host branch August 2, 2022 21:45
@github-actions github-actions bot mentioned this pull request Aug 2, 2022
Merged
@nlf nlf mentioned this pull request Aug 3, 2022
Merged
@npm-cli-bot npm-cli-bot mentioned this pull request Aug 3, 2022
Merged
@gethinwebster gethinwebster mentioned this pull request Jan 11, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wraithgar wraithgar Awaiting requested review from wraithgar

2 more reviewers

@ljharb ljharb ljharb left review comments

@nlf nlf nlf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver:minor new backwards-compatible feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@fritzy @npm-robot @everett1992 @darcyclarke @npm-cli-bot @wraithgar @ljharb @nlf