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: set default max concurrency to 100 for web-api client #1583

Merged
merged 1 commit into from Jan 24, 2023
Merged

feat: set default max concurrency to 100 for web-api client #1583

merged 1 commit into from Jan 24, 2023

Conversation

callumsteele4
Copy link
Contributor

@callumsteele4 callumsteele4 commented Jan 23, 2023

Summary

Fixes #1582

Sets the default maxRequestionConcurrency to 100 and updates relevant documentation and tests.

I've been unable to get the tests and linting running locally despite running npm i at root, npm run setup at root, and then npm run test at either root or in the specific package - so will be relying on GitHub actions to verify these pass.

Requirements (place an x in each [ ])

@CLAassistant
Copy link

CLAassistant commented Jan 23, 2023

CLA assistant check
All committers have signed the CLA.

@mwbrooks mwbrooks requested a review from seratch January 23, 2023 21:25
@mwbrooks mwbrooks added enhancement M-T: A feature request for new functionality semver:minor and removed semver:minor labels Jan 23, 2023
@mwbrooks
Copy link
Member

Beauty @callumsteele4 🎉 🙇🏻 Thanks for following up with a pull request on #1582!

Your pull request looks good, so I've enabled the CI/CD tests to run on it.

@seratch I've assigned you as a reviewer, since you were talking with @callumsteele4 on the issue. I haven't chosen a semver or assigned it to a release milestone. I suspect this could be considered a patch, but I'll trust you to make that decision.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

The doc and code changes look great to me! but the tests are failing. Could you resolve it?

packages/web-api/src/WebClient.spec.js Show resolved Hide resolved
@seratch seratch added the pkg:web-api applies to `@slack/web-api` label Jan 23, 2023
@seratch seratch added this to the web-api@6.8.1 milestone Jan 23, 2023
@seratch
Copy link
Member

seratch commented Jan 23, 2023

@mwbrooks Thanks for checking this!

@seratch I've assigned you as a reviewer, since you were talking with @callumsteele4 on the issue. I haven't chosen a semver or assigned it to a release milestone. I suspect this could be considered a patch, but I'll trust you to make that decision.

I agree that we can include this in a patch version

@callumsteele4 callumsteele4 requested review from seratch and mwbrooks and removed request for seratch and mwbrooks January 24, 2023 10:59
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

LGTM

@seratch seratch merged commit acef8cf into slackapi:main Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClientOptions.maxRequestConcurrency default value and types
5 participants