-
Notifications
You must be signed in to change notification settings - Fork 572
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(fetch): allow setting base urls #1631
Conversation
Is this change even wanted, or does it add ambiguity? I believe Deno offers a cli flag for doing something similar. Also, if a WPT test runner is ever added, will allow a majority of tests to run. |
Codecov Report
@@ Coverage Diff @@
## main #1631 +/- ##
==========================================
+ Coverage 94.51% 94.95% +0.43%
==========================================
Files 49 51 +2
Lines 4799 4814 +15
==========================================
+ Hits 4536 4571 +35
+ Misses 263 243 -20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it 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.
OK with me.
lib/fetch/global.js
Outdated
'use strict' | ||
|
||
/** @type {undefined|URL} */ | ||
let origin |
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 would use the same logic setGlobalDispatcher uses for this.
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.
Is it fine to add another symbol? Wasn't sure because of the issue linked above
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.
Sure thing.
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.
done
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
* feat(fetch): allow setting base url * add files :| * fix: set origin globally * fix: add docs
* feat(fetch): allow setting base url * add files :| * fix: set origin globally * fix: add docs
Notes:
Will not work between different undici versions (assetGlobalDispatcher
/getGlobalDispatcher
do), because of undici leaking Symbol to global space in 18.2 (but not 18.1) node#43157.http:
orhttps:
urls.fetch
calls. This could come later, but I'd rather not add a million undici-only keys to RequestInit.Usage: