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

refactor(site): use axios in non-swr endpoints #460

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Conversation

greyscaled
Copy link
Contributor

Summary

Applies axios to login, logout and getApiKey

Impact

POC of axios (#453) and testing axios

Additional details

  • add test:watch script (port from v1 for improved test workflow)

resolves: #453

Summary:

Applies axios to login, logout and getApiKey

Impact:

POC of axios (#453) and testing axios

Additional details:

* add test:watch script

resolves: #453
@greyscaled greyscaled self-assigned this Mar 16, 2022
@greyscaled greyscaled requested a review from a team as a code owner March 16, 2022 09:28
@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #460 (df55729) into main (95160d0) will increase coverage by 0.42%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #460      +/-   ##
==========================================
+ Coverage   67.74%   68.17%   +0.42%     
==========================================
  Files         160      157       -3     
  Lines        9265     9078     -187     
  Branches       83       79       -4     
==========================================
- Hits         6277     6189      -88     
+ Misses       2363     2288      -75     
+ Partials      625      601      -24     
Flag Coverage Δ
unittest-go-macos-latest 62.69% <ø> (+0.14%) ⬆️
unittest-go-ubuntu-latest 67.54% <ø> (+0.16%) ⬆️
unittest-go-windows-2022 ?
unittest-js 63.64% <100.00%> (+1.49%) ⬆️
Impacted Files Coverage Δ
site/src/api.ts 54.83% <100.00%> (+31.15%) ⬆️
pty/ptytest/ptytest.go 94.44% <0.00%> (-5.56%) ⬇️
provisionerd/provisionerd.go 77.57% <0.00%> (-0.19%) ⬇️
pty/start_windows.go
agent/usershell/usershell_windows.go
pty/pty_windows.go
coderd/provisionerdaemons.go 58.15% <0.00%> (+1.08%) ⬆️
peer/conn.go 78.26% <0.00%> (+1.27%) ⬆️
provisioner/echo/serve.go 57.50% <0.00%> (+2.50%) ⬆️
peerbroker/dial.go 82.45% <0.00%> (+7.01%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95160d0...df55729. Read the comment docs.

Comment on lines +116 to +118
const payload = JSON.stringify({
email,
password,
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how streamlined these are - nice refactoring!

@greyscaled greyscaled enabled auto-merge (squash) March 16, 2022 16:46
@greyscaled greyscaled merged commit 810b2d2 into main Mar 16, 2022
@greyscaled greyscaled deleted the vapurrmaid/453 branch March 16, 2022 17:01
@presleyp
Copy link
Contributor

Such an improvement! If we keep going with axios, what do you think about using an instance or defaults to cut down on boilerplate even more?

@greyscaled
Copy link
Contributor Author

Such an improvement! If we keep going with axios, what do you think about using an instance or defaults to cut down on boilerplate even more?

I like the idea of instance, as long as it's not something that makes debugging or testing harder to understand.

I can imagine a dir structure like this (just thinking out loud):

services/
  coderapi.ts
  other.ts

where each file defines its own axios instance and functions:

// coderapi.ts
export const CoderAPI = axios.create( ... )

export const login = (....) => {
  const response = CoderAPI.post(...)
  return response.data
}

@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch from fetch to axios
4 participants