-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(publish): enable throttling when publishing modules #4013
Conversation
@Ayowel this comment wasn't entirely clear to me:
Are you going to be reporting back to us once you have done this? Is that repo in question a real thing? Or something built just to test this functionality? I have pulled in the latest main so that you can get initial CI feedback |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 1e38501. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 8 targets
Sent with 💌 from NxCloud. |
It failed code formatting, you can run |
The repo is a real thing, I'm having trouble with the specifics of lerna's configuration as some configuration works when testing locally on a small repo but not when pushing with the actual repository. It appears that the NPM limit is not fixed (I was at 25 publish per day before getting timed-out initially, I'm allowed ~500/day at the moment and I'm not clear why the limit changed), so I'll move throttling to an off-by-default state to honor the principle of least astonishment (and enabling throttling will be the user's responsibility). I'll rebase, format, and push once I figure this out |
acd2760
to
376dcc0
Compare
@JamesHenry Looks good to me, removing draft status. {
"command": {
"publish": {
"throttle": true,
"throttleDelay": 7200,
"throttleSize": 40
}
}
} |
376dcc0
to
6d5a2c4
Compare
Not sure if I should keep rebasing each time master is updated, let me know if i shouldn't |
…ault throttle pool size to 25
7bf96d3
to
9ae8845
Compare
9ae8845
to
1e38501
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @Ayowel!
The aim is to provide a way to fix #3962 .
This is opened to validate the implementation design and get feedback on the specific values that should be used as defaults.
Currently known limits when uploading to NPM:
Description
Add three publishing options:
--throttle
to enable/disable throttling--throttle-size
: the 'bucket size' to use when throttling--throttle-delay
: Once the bucket is full, the minimum time between the end of a module upload and the start of an other module's uploadThrottling is enabled by default when publishing to NPM. The throttling implementation is designed to allow changes in the implementation/maintaining different implementations without too much trouble
Motivation and Context
See #3962: NPM throttles module publishing, which results in partial publish which are a pain to deal with
How Has This Been Tested?
Tested on ubuntu with node 20.9.0:
libs/commands/publish/src/lib/throttle-queue.spec.ts
The removal from draft of this PR will wait for the upload to NPM of a 10000+ modules monorepo
Types of changes
Checklist: