-
Notifications
You must be signed in to change notification settings - Fork 1k
optimization: When using custom SSE request,Authorization
header can still be automatically attached to the SSE request.
#478
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
base: main
Are you sure you want to change the base?
Conversation
…header can still be automatically attached to the SSE request.
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.
Hi @chenxi-null, thanks for sending this out! I took the liberty to refactor / simplify the code a bit, let me know your thoughts :-)
.gitignore
Outdated
@@ -120,6 +120,7 @@ out | |||
|
|||
# Stores VSCode versions used for testing VSCode extensions | |||
.vscode-test | |||
.vscode/ |
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.
Not sure this belongs here?
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.
I want to make the .vscode folder untracked by Git.
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.
There's now some conflicts coming in from main onto the tests, could you have a look?
@ochafik Okay, I'll resolve the conflicts. |
…sh-token # Conflicts: # src/client/sse.test.ts # src/client/sse.ts
@ochafik I have resolved the code conflicts. By the way, I find a test case failure which is already existed in main branch. So I can't pass all test cases, but it's caused by the other code in main branch. It need someone else to fix it.
|
When using the custom SSE request, the
Authorization
header can still be automatically attached to the SSE request.Motivation and Context
Current:
When user customizes the initial SSE request, it will prevent an
Authorization
header from being automatically attached to the SSE request. It require user to fetch tokens fromauthProvider
and set theAuthorization
header manually.Ref:
typescript-sdk/src/client/sse.ts
Lines 36 to 44 in bced33d
Goal:
Simplify the code and reduce the risk of misuse.
There is an misused case: CherryHQ/cherry-studio#5709
How Has This Been Tested?
npm test -- src/client/sse.test.ts -t "refreshes expired token during SSE connection"
Breaking Changes
Types of changes
Checklist
Additional context