-
Notifications
You must be signed in to change notification settings - Fork 32
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
Chore: Remove axios as a dependency for sign-plugin #292
Conversation
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.
LGTM! Added one small nit. No need to mark the function with async since you are returning a promise.
status: number; | ||
} | ||
|
||
export async function postData(urlString: string, data: unknown, headers: Headers): Promise<Response<string>> { |
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.
export async function postData(urlString: string, data: unknown, headers: Headers): Promise<Response<string>> { | |
export function postData(urlString: string, data: unknown, headers: Headers): Promise<Response<string>> { |
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.
Good point! Usually the linter catches these things. I'll see if I can modify the rules so this is automatically suggested.
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.
Awesome! We might also want to get rid of minimist next and have 0 deps.
🚀 PR was released in |
What this PR does / why we need it:
Removes axios as a dependency for sign-plugin. Axios is only used to make a single post request and it adds a layer of "black box" that can make situations like #290 harder to diagnostic.
Test run with a success case
Test run with an error case
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
📦 Published PR as canary version:
Canary Versions
✨ Test out this PR locally via:
npm install @grafana/sign-plugin@1.0.3-canary.292.c5013c7.0 # or yarn add @grafana/sign-plugin@1.0.3-canary.292.c5013c7.0