Skip to content

feat(web-api): add request interceptor and HTTP adapter config to WebClient [ISSUE-2073]#2076

Merged
filmaj merged 10 commits intoslackapi:mainfrom
mtjandra:issue-2073/axios-interceptors
Oct 16, 2024
Merged

feat(web-api): add request interceptor and HTTP adapter config to WebClient [ISSUE-2073]#2076
filmaj merged 10 commits intoslackapi:mainfrom
mtjandra:issue-2073/axios-interceptors

Conversation

@mtjandra
Copy link
Copy Markdown
Contributor

@mtjandra mtjandra commented Oct 11, 2024

Summary

Resolves #2073

Features

  • ability to configure one axios request interceptor
    • useful for clients who would like to manipulate outgoing requests to conform to their custom proxies
  • ability to configure an axios adapter
    • useful for client who would like to used their existing http client
      • ex: client may have a dedicated http client configured to be used with their internal proxy

Contribution Pre-Requisites

@salesforce-cla
Copy link
Copy Markdown

salesforce-cla Bot commented Oct 11, 2024

Thanks for the contribution! Before we can merge this, we need @mtjandra to sign the Salesforce Inc. Contributor License Agreement.

@mtjandra mtjandra mentioned this pull request Oct 11, 2024
Closed
@filmaj filmaj self-requested a review October 15, 2024 15:36
@filmaj filmaj closed this Oct 15, 2024
@filmaj filmaj reopened this Oct 15, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.23656% with 7 lines in your changes missing coverage. Please review.

Project coverage is 91.83%. Comparing base (2af216c) to head (5dabe0f).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2076      +/-   ##
==========================================
+ Coverage   91.75%   91.83%   +0.07%     
==========================================
  Files          38       38              
  Lines       10093    10264     +171     
  Branches      631      646      +15     
==========================================
+ Hits         9261     9426     +165     
- Misses        820      826       +6     
  Partials       12       12
Flag Coverage Δ
cli-hooks 95.23% <ø> (ø)
cli-test 95.69% <ø> (ø)
oauth 77.39% <ø> (ø)
socket-mode 58.22% <ø> (ø)
web-api 96.88% <96.23%> (-0.02%) ⬇️
webhook 96.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

filmaj
filmaj suggested changes Oct 15, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Thanks so much for the PR! I think this is heading in a great direction. I left many questions for my own education and to help inform my future reviews as well.

One higher-level question: what is the intention behind exposing both an adapter option as well as requestInterceptor? How/why would consumers use either/or? Would they use both in any situation? The request interceptor makes sense to expose, but based on the axios docs, it seems adapter is mainly for use for testing (https://axios-http.com/docs/req_config)?

Also, I think we may need to add something to our docs about this new feature. Looking at the right menu bar for the web-api docs here, perhaps a new section on request interceptors / modifiers would be in order? What do you think?

Comment thread packages/web-api/src/WebClient.ts
Comment thread packages/web-api/src/WebClient.ts
Comment thread packages/web-api/src/WebClient.ts
Comment thread packages/web-api/src/WebClient.ts Outdated
Comment thread packages/web-api/src/WebClient.spec.ts Outdated
Comment thread packages/web-api/src/WebClient.spec.ts Outdated
@filmaj filmaj added this to the [email protected] milestone Oct 15, 2024
@filmaj filmaj added semver:minor enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` labels Oct 15, 2024
@mtjandra mtjandra requested a review from filmaj October 16, 2024 14:53
filmaj
filmaj approved these changes Oct 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Awesome work! Left a few minor documentation suggestions; once applied or rejected, I will merge this in.

Thank you so much for your work here and patience with me during review, it is greatly appreciated 🙇

Comment thread docs/content/packages/web-api.md Outdated
Comment thread docs/content/packages/web-api.md Outdated
Comment thread docs/content/packages/web-api.md Outdated
Comment thread docs/content/packages/web-api.md Outdated
Comment thread packages/web-api/src/WebClient.spec.ts Outdated
Comment thread packages/web-api/src/WebClient.ts Outdated
Comment thread packages/web-api/src/WebClient.ts
Comment thread packages/web-api/src/WebClient.ts Outdated
@mtjandra mtjandra force-pushed the issue-2073/axios-interceptors branch 5 times, most recently from 1c5eec0 to 2652028 Compare October 16, 2024 19:41
@mtjandra mtjandra force-pushed the issue-2073/axios-interceptors branch from 2652028 to 5dabe0f Compare October 16, 2024 19:45
@mtjandra
Copy link
Copy Markdown
Contributor Author

mtjandra commented Oct 16, 2024

Awesome work! Left a few minor documentation suggestions; once applied or rejected, I will merge this in.

Thank you so much for your work here and patience with me during review, it is greatly appreciated 🙇

Thank you for helping me with this and being responsive! I've addressed your comments and have rebased with main

@filmaj filmaj merged commit 8ba3a43 into slackapi:main Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@filmaj filmaj filmaj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla:signed enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` semver:minor

Projects

None yet

Milestone

[email protected]

Development

Successfully merging this pull request may close these issues.

web-api: expose axios interceptors as WebClient constructor option

2 participants

@mtjandra @filmaj