-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: implement client fallback mechanism #16
Changes from all commits
804978c
f4d1844
894d16d
7009c7a
3336a87
696cd8a
a3dec56
747d780
8dec1c7
b0dc673
78651fc
d15c0ad
29e8da1
194ce70
1d97dc9
84a1b8e
f688406
5b7c215
6068a90
2c3cc79
4cf0c00
d81038f
a1ecd50
55d56d1
d2c5c37
e0065d8
109019b
b84ce4a
7731fe8
e0fcfd8
ac1b9f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// @ts-check | ||
|
||
/** | ||
* | ||
* @param {URL} url | ||
* @returns {URL|string} | ||
*/ | ||
export function parseUrl (url) { | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @guanzo had to add this try catch here because when testing in the browser, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why's it being imported by the browser-client if its only used for testing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry its not only for testing, parseUrl is used within the js-client code. Im was just saying the reason it exists because the mock tests don't accept |
||
// This is a temp function to resolve URLs for mock testing | ||
// See issue here: https://github.com/mswjs/msw/issues/1597 | ||
if (process?.env?.TESTING) { | ||
return url.toJSON() | ||
} | ||
} catch (e) {} | ||
|
||
return url | ||
} | ||
|
||
/** | ||
* | ||
* @param {string} url | ||
* @returns {string} | ||
*/ | ||
export function addHttpPrefix (url) { | ||
// This is a temp function to resolve URLs for mock testing | ||
// See issue here: https://github.com/mswjs/msw/issues/1597 | ||
if (!url.startsWith('http')) { | ||
url = `https://${url}` | ||
} | ||
return url | ||
} |
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.
@guanzo I want to clean this up a little by having those URLs in one place and they get imported here. You cool with that?
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.
agree on this if possible -- having these really weird URLs hardcoded right in the code is a bit odd. I might this a private static class readonly var "defaultConfig" or something
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.
Do we want to keep our auth URL at least a private var?
Doesn't completely hide it but at least makes it harder for someone to find it and use it.
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.
These kinds of URL config vars should typically be env vars injected during the build process.
When you guys say "private var" do you mean https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields?
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.
yes thats correct.