-
Notifications
You must be signed in to change notification settings - Fork 146
fix: bug in using API_BASE with URL #1228
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
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1228 +/- ##
==========================================
+ Coverage 84.01% 84.20% +0.18%
==========================================
Files 68 68
Lines 2947 2938 -9
Branches 374 374
==========================================
- Hits 2476 2474 -2
+ Misses 411 404 -7
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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'm ok with dropping the unused query args (which may need to come back one day for server-side pagination or filtering). However, I think API_BASE needs to fall back to location.origin
.
I'll raise an issue for the remaining UI services to be converted to TS - they should also use API_BASE (instead of each working it out themselves), but will need the location.origin fallback (I guess).
@kriswest Ok - I'm fine with defaulting to |
The other services currently default to location.origin - I didn't notice user.ts had lost that in the original PR (but was happy to see the hardcoded |
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
Summary
This PR fixes a bug introduced in #1201 related to the URL API.
The
URL
constructor does not support relative paths. Whenprocess.env.VITE_API_URI
is not defined,API_BASE
falls back to a relative path. As a result, code like this throws an exception in certain environments:Fix
URL
objects when building url string with${API_BASE}
./api/v1/user
does not pass any query params.getUsers
always passed an empty query params object.UserList