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

feat(fetch): allow setting base urls #1631

Merged
merged 4 commits into from
Sep 2, 2022
Merged

Conversation

KhafraDev
Copy link
Member

@KhafraDev KhafraDev commented Sep 2, 2022

Notes:

  1. Will not work between different undici versions (as setGlobalDispatcher/getGlobalDispatcher do), because of undici leaking Symbol to global space in 18.2 (but not 18.1) node#43157.
  2. Only allows http: or https: urls.
  3. Allows strings, URL objects, or undefined, as ts lists all of them as a second parameter for the URL constructor.
  4. Setting undefined resets the base url (relative paths will throw again).
  5. No way to set for individual fetch calls. This could come later, but I'd rather not add a million undici-only keys to RequestInit.

Usage:

import { setGlobalOrigin, getGlobalOrigin } from 'undici'

getGlobalOrigin() // undefined

setGlobalOrigin('http://localhost:3000')

getGlobalOrigin() // equal to new URL('http://localhost:3000')

@KhafraDev KhafraDev changed the title Relative url feat(fetch): allow setting base urls Sep 2, 2022
@KhafraDev
Copy link
Member Author

KhafraDev commented Sep 2, 2022

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-commenter
Copy link

codecov-commenter commented Sep 2, 2022

Codecov Report

Merging #1631 (cf89498) into main (08772d9) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)
lib/fetch/global.js 100.00% <100.00%> (ø)
lib/fetch/request.js 85.30% <100.00%> (+0.06%) ⬆️
lib/fetch/response.js 94.28% <100.00%> (+0.03%) ⬆️
lib/fetch/util.js 86.30% <0.00%> (-0.28%) ⬇️
lib/llhttp/llhttp.wasm.js 100.00% <0.00%> (ø)
lib/fetch/file.js 92.94% <0.00%> (+0.08%) ⬆️
lib/core/request.js 96.89% <0.00%> (+0.62%) ⬆️
lib/core/util.js 97.46% <0.00%> (+1.26%) ⬆️
lib/client.js 96.98% <0.00%> (+1.70%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

OK with me.

'use strict'

/** @type {undefined|URL} */
let origin
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@KhafraDev KhafraDev merged commit 9ab4967 into nodejs:main Sep 2, 2022
@KhafraDev KhafraDev deleted the relative-url branch September 2, 2022 18:06
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
* feat(fetch): allow setting base url

* add files :|

* fix: set origin globally

* fix: add docs
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* feat(fetch): allow setting base url

* add files :|

* fix: set origin globally

* fix: add docs
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.

None yet

4 participants