-
Notifications
You must be signed in to change notification settings - Fork 5.8k
refactor: merge deno_fetch into deno_web
#31339
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe PR removes the ext/fetch crate and its workspace entries and replaces deno_fetch usages across the codebase with deno_web::fetch equivalents. It adds a new ext/web/fetch module implementing a full HTTP client (ReqBody/ResBody, Client, errors, ops, resources, DNS/proxy/file handlers) and updates JS imports and internal d.ts to reference ext:deno_web/*. Workspace Cargo.toml entries and runtime extension initialization were adjusted to initialize and expose the new deno_web fetch functionality. Sequence Diagram(s)sequenceDiagram
autonumber
participant JS as Runtime JS (global fetch)
participant Op as Op Layer (op_fetch / op_fetch_send)
participant ExtWeb as ext:deno_web (fetch ops)
participant Client as deno_web::fetch::Client
participant Backend as Network/File/DataURL
JS->>Op: fetch(request)
Op->>ExtWeb: validate & toInnerRequest
ExtWeb->>Client: get_or_create_client / send(http::Request<ReqBody>)
Client->>Backend: perform network/file/data fetch (async)
Backend-->>Client: http::Response<ResBody> / error
Client-->>ExtWeb: response (ResBody)
ExtWeb->>Op: wrap into FetchResponseResource / stream body
Op-->>JS: resolve Promise with Response (streaming)
rect rgb(220,240,230)
Note right of Client: retry, TLS, proxy, pooling, client certs
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Points to focus review on:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (9)
ext/fetch/README.md (1)
3-4: Deprecation notice is clear; consider adding migration guidance (optional).The deprecation message effectively communicates the transition to
deno_web. For enhanced user experience, consider adding a brief note with migration guidance or a link to release notes/docs that explain how to update imports and usage patterns, especially for projects with existingdeno_fetchdependencies.Example enhancement:
# deno_fetch -This crate has been deprecated, use -[deno_web](https://crates.io/crates/deno_web) instead. +This crate has been deprecated, use +[deno_web](https://crates.io/crates/deno_web) instead. + +For migration guidance, see the [deno_web fetch documentation](https://docs.rs/deno_web/latest/deno_web/fetch/).ext/web/internal.d.ts (1)
3-4: Fetch-related internal typings are aligned, butFormDataalias likely targets the wrong type
- Declaring
domIterable.DomIterableMixin, the headers helpers,InnerBody, and the inner request/response conversion APIs underext:deno_web/*matches how the JS modules are structured and gives the TS checker enough shape information for these internals.- Inside the
ext:deno_web/21_formdata.jsdeclaration,type FormData = typeof FormData;makes theFormDataalias refer to the constructor type, but functions likeformDataToBlob(formData: FormData)andInnerBody.source: Blob | FormDataconceptually operate onFormDatainstances, not the constructor.- Unless this is intentional, consider changing that alias to the instance type (for example,
type FormData = globalThis.FormData;) to keep parameter and field types accurate.Please verify that the TS checker is happy with this alias when type-checking
ext/web/21_formdata.jsand its consumers; if you see confusing assignability errors aroundFormData, adjusting the alias as suggested should resolve them.Also applies to: 144-147, 148-170, 172-182, 184-203, 205-236
cli/http_util.rs (1)
19-22: CLI HTTP client successfully migrated todeno_web::fetch
HttpClientProvidernow creates and cachesdeno_web::fetch::Clientinstances viacreate_http_clientandCreateHttpClientOptions, keeping configuration (root cert store,unsafely_ignore_certificate_errors) identical to the previousdeno_fetch-based setup.SendErrorandDownloadErrorKind::Fetchcorrectly wrapdeno_web::fetch::ClientSendError, so error propagation throughDownloadErrorremains intact.HttpClient::{get,post,post_json,send}usingdeno_web::fetch::ReqBody::{empty,full}and returninghttp::Response<ResBody>/http::Response<deno_web::fetch::ResBody>preserves the previous request/response behavior while aligning with the new body types.get_redirected_responseandget_response_body_with_progressnow operate on the newResBodybut retain the existing redirect, progress, and error-handling semantics, and the TLS-focused tests at the bottom of the file still exercisecreate_http_clientvia the new imports.You might optionally import
deno_web::fetch::{Client, ReqBody, ResBody}and use those aliases consistently (e.g., inget_redirected_responseandRequestBuilder) to reduce repetition. Please also run the existing HTTP/TLS tests in this module to confirm the behavior is unchanged with the new client implementation.Also applies to: 35-36, 46-47, 113-114, 179-181, 191-196, 199-200, 211-212, 231-232, 239-255, 326-331, 375-376, 443-445, 453-460
ext/web/fetch/mod.rs (3)
227-256: Prefer a scoped shared-borrow when cloningfile_fetch_handlerIn the
"file"branch ofop_fetch(around Line 408), you currently destructureOptionsfromstate.borrow_mut::<Options>()and then immediately callfile_fetch_handler.fetch_file(state, &url)on the sameOpState. While the compiler enforces safety, it would be clearer (and future‑proof against additional borrows insidefetch_file) to take a short‑lived shared borrow, clone the handler, and then drop the borrow before calling into it.Consider refactoring along these lines:
- let Options { - file_fetch_handler, .. - } = state.borrow_mut::<Options>(); - let file_fetch_handler = file_fetch_handler.clone(); + let file_fetch_handler = { + let options = state.borrow::<Options>(); + options.file_fetch_handler.clone() + }; let (future, maybe_cancel_handle) = file_fetch_handler.fetch_file(state, &url);This avoids mixing a borrow of
Optionswith a&mut OpStateuse in the same logical block and makes the aliasing story more obviously sound.Also applies to: 408-421
308-375: Re‑check the single‑threaded invariant forResourceToBodyAdapter
ResourceToBodyAdapter’s stream implementation and chunking logic look fine, but theunsafe impl Send/unsafe impl Syncrely entirely on the comment that it is “only used on a single-threaded executor”. Given this type now sits in the consolidatedext/web/fetchmodule and is used as ahyper::body::Body, it would be good to periodically validate that:
- This body never crosses into a multithreaded executor or thread‑pool context, and
- Future refactors to the HTTP client don’t change that assumption.
If there’s any chance this might be used across threads later, consider either removing the unsafe impls (and plumbing a non‑
Sendbody where possible) or adding a stronger guard/doc comment explaining where this is instantiated.
813-887: HTTP client creation and TLS/proxy wiring mostly look good; avoidunwrap()on TLS keysThe
op_fetch_custom_client+CreateHttpClientOptions+create_http_clientpipeline is generally well‑designed:
- Proxy variants are validated against permissions (
check_net_url,check_net,check_open,check_net_vsock) before use.- TLS config is built via
deno_tls::create_client_configwith root store, CA certs, and unsafe options aggregated in one place.- ALPN handling for proxy vs end‑server,
http1/http2flags, pool limits, idle timeouts, andlocal_addressparsing are all coherently wired.- CLI/JS‑level proxy options correctly override environment proxies by using
proxies.prepend(intercept).One minor concern is the use of:
client_cert_chain_and_key: tls_keys.take().try_into().unwrap(),If
try_into()can ever fail (for example due to invalid or already-consumed keys), this will panic and bring down the runtime instead of surfacing a structuredHttpClientCreateError. If such failure is truly impossible by construction, a brief comment to that effect would make the invariant clearer; otherwise, mapping the error into a newHttpClientCreateErrorvariant would be safer.Everything else in this area looks consistent with the earlier
deno_fetchbehavior after consolidation.Also applies to: 894-951, 953-1094
runtime/web_worker.rs (1)
523-541: Web workers now initialize fetch viadeno_web::fetch::OptionsPassing
user_agent,root_cert_store_provider,unsafely_ignore_certificate_errors, and theFsFetchHandlerintodeno_web::deno_web::initcleanly hooks web workers into the unified fetch pipeline (includingfile:support) and matches the newext/web::fetchAPI.If the same
fetch::Optionsblock is duplicated in the main worker setup, consider a small shared helper to construct it so future tweaks (e.g., new hooks) stay in sync across runtimes.cli/registry.rs (1)
90-92: Registry HTTP helpers now usedeno_web::fetch::ResBodyconsistentlyUpdating
parse_responseandget_packageto work withdeno_web::fetch::ResBodybrings the CLI registry code in line with the new fetch plumbing; the surrounding logic remains compatible.
http_util::body_to_string(response).await.unwrap()will still panic on body read failures. Not introduced by this PR, but if you revisit this path, consider propagating that error into anApiError(orAnyErrorat the call site) instead of unwrapping.Also applies to: 135-141
ext/kv/remote.rs (1)
172-197: Optional: align TLS key conversion withext/web/fetch::create_client_from_optionsThe switch to
deno_web::fetch::create_http_clientandCreateHttpClientOptionslooks correct: TLS, proxy, DNS, HTTP/2‑only, and ignore‑cert flags all map cleanly onto the new struct and match howext/web/fetchbuilds clients.One small robustness tweak you might consider is aligning
client_cert_chain_and_keyhandling withext/web/fetch::create_client_from_options, which usesunwrap_or_default()instead ofunwrap()on thetry_into()result. That would avoid a potential panic if the conversion ever fails and keep behavior consistent with the main fetch path.- client_cert_chain_and_key: options - .client_cert_chain_and_key - .clone() - .try_into() - .unwrap(), + client_cert_chain_and_key: options + .client_cert_chain_and_key + .clone() + .try_into() + .unwrap_or_default(),It’s worth double‑checking that the
TryIntotarget type here matches the one used inext/web/fetch::CreateHttpClientOptionsand thatDefaultis implemented as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktests/specs/run/wasm_streaming_panic_test/wasm_streaming_panic_test.js.outis excluded by!**/*.out
📒 Files selected for processing (59)
Cargo.toml(0 hunks)cli/Cargo.toml(1 hunks)cli/http_util.rs(13 hunks)cli/registry.rs(2 hunks)cli/tools/publish/mod.rs(1 hunks)cli/tools/test/mod.rs(1 hunks)ext/cache/01_cache.js(1 hunks)ext/cache/Cargo.toml(1 hunks)ext/cache/lib.rs(1 hunks)ext/fetch/Cargo.toml(1 hunks)ext/fetch/README.md(1 hunks)ext/fetch/internal.d.ts(0 hunks)ext/fetch/lib.rs(0 hunks)ext/http/00_serve.ts(1 hunks)ext/http/01_http.js(1 hunks)ext/http/02_websocket.ts(1 hunks)ext/http/Cargo.toml(1 hunks)ext/http/lib.rs(2 hunks)ext/kv/Cargo.toml(1 hunks)ext/kv/lib.rs(1 hunks)ext/kv/remote.rs(3 hunks)ext/net/README.md(0 hunks)ext/node/Cargo.toml(1 hunks)ext/node/lib.rs(1 hunks)ext/node/ops/http.rs(3 hunks)ext/node/polyfills/http.ts(1 hunks)ext/node/polyfills/http2.ts(1 hunks)ext/node/polyfills/https.ts(1 hunks)ext/process/40_process.js(1 hunks)ext/web/22_body.js(1 hunks)ext/web/23_request.js(2 hunks)ext/web/23_response.js(2 hunks)ext/web/26_fetch.js(1 hunks)ext/web/27_eventsource.js(1 hunks)ext/web/Cargo.toml(1 hunks)ext/web/benches/encoding.rs(1 hunks)ext/web/benches/timers_ops.rs(1 hunks)ext/web/benches/url_ops.rs(1 hunks)ext/web/fetch/fs_fetch_handler.rs(1 hunks)ext/web/fetch/mod.rs(1 hunks)ext/web/fetch/tests.rs(2 hunks)ext/web/internal.d.ts(2 hunks)ext/web/lib.rs(4 hunks)ext/websocket/01_websocket.js(1 hunks)ext/websocket/02_websocketstream.js(1 hunks)ext/websocket/Cargo.toml(1 hunks)ext/websocket/lib.rs(4 hunks)runtime/Cargo.toml(0 hunks)runtime/js/90_deno_ns.js(1 hunks)runtime/js/98_global_scope_shared.js(1 hunks)runtime/js/99_main.js(1 hunks)runtime/lib.rs(0 hunks)runtime/ops/web_worker/sync_fetch.rs(3 hunks)runtime/shared.rs(0 hunks)runtime/snapshot.rs(0 hunks)runtime/snapshot_info.rs(1 hunks)runtime/web_worker.rs(1 hunks)runtime/worker.rs(2 hunks)tools/core_import_map.json(1 hunks)
💤 Files with no reviewable changes (8)
- ext/net/README.md
- runtime/snapshot.rs
- runtime/lib.rs
- Cargo.toml
- runtime/Cargo.toml
- runtime/shared.rs
- ext/fetch/lib.rs
- ext/fetch/internal.d.ts
🧰 Additional context used
🧬 Code graph analysis (12)
ext/node/ops/http.rs (2)
ext/web/fetch/mod.rs (1)
extract_authority(1348-1374)ext/web/fetch/proxy.rs (1)
basic_auth(185-203)
cli/tools/publish/mod.rs (1)
ext/web/fetch/mod.rs (1)
full(1290-1292)
runtime/web_worker.rs (3)
ext/web/fetch/mod.rs (1)
user_agent(995-995)cli/factory.rs (2)
root_cert_store_provider(440-448)new(123-134)cli/args/mod.rs (3)
unsafely_ignore_certificate_errors(1220-1222)new(366-376)new(490-516)
runtime/worker.rs (4)
ext/web/fetch/mod.rs (1)
user_agent(995-995)cli/factory.rs (3)
root_cert_store_provider(440-448)new(123-134)resolver(682-685)cli/lib/worker.rs (1)
new(534-578)ext/node/ops/dns.rs (1)
resolver(43-48)
ext/kv/remote.rs (2)
ext/web/fetch/mod.rs (2)
full(1290-1292)create_http_client(955-1094)cli/tsc/dts/lib.deno.ns.d.ts (1)
CreateHttpClientOptions(6305-6336)
ext/websocket/lib.rs (1)
ext/web/fetch/mod.rs (1)
get_or_create_client_from_state(265-276)
ext/web/lib.rs (2)
ext/web/26_fetch.js (1)
op_fetch(174-182)ext/web/fetch/mod.rs (5)
op_fetch(381-555)op_fetch_send(575-635)op_utf8_to_byte_string(1098-1100)op_fetch_custom_client(812-892)op_fetch_promise_is_settled(1377-1379)
runtime/ops/web_worker/sync_fetch.rs (1)
ext/web/fetch/mod.rs (3)
create_client_from_options(278-306)req(1403-1403)empty(1294-1296)
ext/web/fetch/tests.rs (1)
ext/web/fetch/mod.rs (1)
empty(1294-1296)
ext/web/internal.d.ts (5)
ext/web/23_response.js (7)
list(223-223)headers(205-205)headers(490-492)body(115-115)status(484-486)headerList(110-113)response(513-513)ext/web/20_headers.js (3)
list(148-148)headers(499-499)entries(166-166)ext/web/22_body.js (4)
body(501-501)boundary(399-399)entries(408-408)stream(433-433)ext/web/26_fetch.js (5)
body(127-127)body(156-156)stream(155-155)response(211-222)response(513-513)ext/web/21_formdata.js (2)
boundary(308-315)entries(362-362)
cli/http_util.rs (2)
cli/tsc/dts/lib.deno.ns.d.ts (1)
CreateHttpClientOptions(6305-6336)ext/web/fetch/mod.rs (8)
create_http_client(955-1094)from(215-224)default(129-141)default(672-675)default(911-926)url(427-429)empty(1294-1296)full(1290-1292)
ext/web/fetch/mod.rs (4)
ext/kv/remote.rs (5)
bytes(110-112)stream(113-117)root_cert_store(37-42)new(50-52)clone(60-64)ext/web/fetch/fs_fetch_handler.rs (1)
fetch_file(31-88)ext/web/fetch/proxy.rs (10)
parse(258-341)parse(967-969)basic_auth(185-203)val(279-279)from_env(140-183)from_env(384-390)intercept(500-502)intercept(519-538)all(212-217)host(443-443)ext/tls/lib.rs (1)
create_client_config(286-361)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release macos-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (49)
ext/web/27_eventsource.js (1)
36-42: Import path migration verified and approved.The updated import paths all resolve correctly—all three modules (
ext/web/20_headers.js,ext/web/23_request.js,ext/web/26_fetch.js) exist in the expected locations. The changes align with the PR objective of mergingdeno_fetchintodeno_web, and all imported functions are actively used in the EventSource implementation.ext/websocket/Cargo.toml (1)
23-23: Dependency migration fromdeno_fetchtodeno_webis correct and complete.Verification confirms that all source code imports in the websocket crate have been properly updated. The crate now imports four types and utilities from
deno_web::fetch(ClientConnectError, HttpClientCreateError, HttpClientResource, and get_or_create_client_from_state), and no lingering references todeno_fetchremain. The Cargo.toml change is fully aligned with the source code implementation.ext/cache/01_cache.js (1)
22-29: Verification confirmed - import migration is complete and correct.All three imported symbols (
toInnerRequest,toInnerResponse,getHeader) are properly exported from their new module locations inext:deno_web, and all usages throughout the file are consistent with the import statements. No remaining references toext:deno_fetchexist in this file.ext/process/40_process.js (1)
31-31: LGTM!The import path update from
ext:deno_fetch/22_body.jstoext:deno_web/22_body.jsis correct and aligns with the PR's objective to migrate fetch functionality intodeno_web.cli/Cargo.toml (1)
96-96: LGTM!Adding
deno_webas a workspace dependency is correct for the migration. This provides the CLI with access to the fetch functionality now consolidated indeno_web.ext/web/22_body.js (1)
50-50: LGTM!The import path update correctly reflects the migration of FormData utilities from
ext:deno_fetch/21_formdata.jstoext:deno_web/21_formdata.js.ext/node/polyfills/https.ts (1)
15-15: LGTM!The import path update from
ext:deno_fetch/22_http_client.jstoext:deno_web/22_http_client.jscorrectly reflects the migration. ThecreateHttpClientfunction usage remains unchanged.ext/cache/lib.rs (1)
101-101: LGTM!The extension dependency update from
deno_fetchtodeno_webis correct and aligns with the migration objectives.ext/kv/lib.rs (1)
38-38: LGTM!Adding the public re-export
pub use deno_web;correctly makes thedeno_webcrate accessible to consumers of theext/kvmodule, supporting the fetch functionality migration.ext/web/fetch/fs_fetch_handler.rs (1)
21-23: LGTM!The import path updates correctly reflect the reorganization of fetch-related types into the
crate::fetchmodule namespace, maintaining consistency with the consolidated fetch structure.cli/tools/test/mod.rs (1)
835-835: LGTM!The type reference update from
deno_runtime::deno_fetch::Clienttodeno_web::fetch::Clientcorrectly reflects the migration while preserving the test isolation logic that ensures each test gets a fresh connection pool.ext/http/Cargo.toml (1)
37-37: LGTM! Dependency migration aligned with PR objective.The removal of
deno_fetchand reliance ondeno_webis consistent with merging fetch functionality intodeno_web.ext/web/benches/url_ops.rs (1)
23-28: LGTM! Consistent with other benchmark files.The fourth argument addition matches the pattern used in other benchmark files.
runtime/js/98_global_scope_shared.js (1)
16-27: LGTM! Import paths consistently updated.All web API imports have been correctly migrated from
ext:deno_fetchtoext:deno_web.ext/fetch/Cargo.toml (1)
5-11: LGTM! Appropriate deprecation of deno_fetch package.The package is correctly marked as deprecated with a clear message directing users to
deno_web. The version bump and removal of dependencies are appropriate for this deprecation.ext/web/benches/timers_ops.rs (1)
29-34: LGTM! Consistent with other benchmark files.The fourth argument addition matches the initialization pattern used throughout the codebase.
ext/web/benches/encoding.rs (1)
30-35: Initialization pattern verified as consistent.The verification confirms that all
deno_web::deno_web::init::<deno_web::InMemoryBroadcastChannel>calls consistently use a four-argument pattern. The change in encoding.rs is consistent with url_ops.rs and timers_ops.rs (all using:Default::default(), None, Default::default(), Default::default()). The variation in snapshot_info.rs appears intentional for that specific module context.ext/web/26_fetch.js (1)
50-59: Migration verification complete—all runtime import paths correctly migrated.The script successfully confirmed no
ext:deno_fetchimport paths remain in JavaScript runtime code. The fileext/web/26_fetch.jsand all related modules correctly useext:deno_web/...imports as shown in the code snippet.runtime/snapshot_info.rs (1)
20-25: Verification complete: extension initialization order is correct.The removal of
deno_fetch::deno_fetch::inithas been consistently applied across all four runtime files. Extension ordering is maintained:
runtime/snapshot_info.rs: telemetry → webidl → web → webgpu → canvas → cache → websocket → webstorageruntime/web_worker.rs: telemetry → webidl → web → (continues)runtime/worker.rs: webidl → web → webgpu → canvasruntime/snapshot.rs: telemetry → webidl → web → webgpu → canvasNo active
deno_fetchreferences exist in Rust runtime code; only TypeScript type definitions and build configuration mention it (expected for compatibility). The migration to consolidate fetch functionality intodeno_webis complete and correct.ext/web/fetch/tests.rs (1)
17-22: Fetch DNS and request body wiring correctly follow newdeno_web::fetchlayout
- Importing
crate::fetch::dnsand usingdns::Resolverinrust_test_client_with_resolveris consistent with the fetch module move.- Updating the TLS fixtures to
"../../tls/testdata/..."matches the new nesting ofext/web/fetch/tests.rsunderext/web/.- Switching to
crate::fetch::ReqBody::empty()aligns the test with the new request body type exposed fromext/web/fetch::ReqBody.Also applies to: 132-135
ext/web/Cargo.toml (1)
18-27: Newdeno_webdependencies match migrated fetch responsibilitiesThe added dependencies (HTTP stack, TLS, DNS, proxy support, and test-only
fast-socks5/rustls) are exactly what the fetch client needs and are correctly marked asworkspace = true(plus target-gatedtokio-vsock). This cleanly absorbsdeno_fetch’s former dependency surface intodeno_web.Also applies to: 29-32, 34-54, 57-59, 63-64
ext/cache/Cargo.toml (1)
24-24:deno_cachenow correctly depends ondeno_webAdding
deno_webas a workspace dependency is consistent with moving fetch primitives there and avoids any lingering reliance ondeno_fetch.ext/websocket/lib.rs (1)
29-33: WebSocket handshake now cleanly reusesdeno_web::fetchclient and errors
- Importing
ClientConnectError,HttpClientCreateError,HttpClientResource, andget_or_create_client_from_statefromdeno_web::fetchkeeps all HTTP client concerns under the consolidated fetch module.HandshakeError::Connect(#[from] ClientConnectError)andWebsocketError::ClientCreate(#[from] HttpClientCreateError)preserve the previous error mapping while swapping to the new types.- Updating
handshake_websocket,handshake_http1, andhandshake_http2to takedeno_web::fetch::Clientis a straightforward type swap; their internal logic and use ofSocketUseremain unchanged.op_ws_createstill respects a passed-in HTTP client resource viaHttpClientResourceand otherwise falls back toget_or_create_client_from_state, so connection reuse andallow_hostsemantics are preserved.- Changing the extension deps to
[deno_web, deno_webidl]matches the new source of the HTTP client and removes the need for a separatedeno_fetchextension.Please ensure the
deno_web::fetchclient still exposesconnect,inject_common_headers, andHttpClientResource { client, allow_host }with the same semantics as before. This should be covered by the existing websocket tests, so a focused websocket test run is a good sanity check.Also applies to: 93-103, 203-246, 248-304, 306-344, 400-427, 848-866
tools/core_import_map.json (1)
513-519: Import map correctly re-homes fetch JS underext:deno_webRemapping the fetch JS modules to
"ext:deno_web/20_headers.js"…"ext:deno_web/26_fetch.js"while keeping the physical paths under../ext/fetch/is a reasonable transitional step and matches the new TypeScript declarations inext/web/internal.d.ts.runtime/worker.rs (1)
200-201: Runtime now configuresdeno_webfetch directly with DNS/TLS options
- Adding
fetch_dns_resolver: deno_web::fetch::dns::ResolvertoWorkerServiceOptionsand passing it intodeno_web::fetch::Optionsmakes the worker’s fetch stack explicitly configurable and aligns with howCreateHttpClientOptionsdefaultsdns::Resolver.- The rest of the
Options(user agent, root cert store provider,unsafely_ignore_certificate_errors, andfile_fetch_handler: FsFetchHandler) mirror the previousdeno_fetch-side configuration, so behavior for network, TLS, and file:// fetches should be preserved.Please double-check that
deno_web::fetch::dns::Resolveris eitherCopyor you never useservicesafter moving this field out; otherwise, consider usingresolver: services.fetch_dns_resolver.clone()here to avoid partial-move issues. A quickcargo checkacrossruntimeshould validate this.Also applies to: 531-549
ext/web/fetch/mod.rs (3)
95-142: Options wiring and defaults look consistentThe
Optionsstruct cleanly encapsulates user agent, TLS roots, proxy, hooks, file handler, and DNS resolver, andDefaultplusroot_cert_store()behavior matches the expected fetch configuration model. No issues spotted here.
376-555: HTTP/HTTPS branch inop_fetchaligns with fetch/network expectationsThe main HTTP/HTTPS path in
op_fetchlooks solid:
- Net permissions are checked via
PermissionsContainer::check_net_urlbefore constructing the URI.- Username/password in the URL are stripped via
extract_authorityand mapped into anAuthorizationheader, avoiding credentials leaking in the request URI.Content-Lengthis set from known body size (buffer or sized resource) and explicitly forced to0forPOST/PUTwith no body, and conflicting userContent-Lengthheaders are ignored.Rangerequests forceAccept-Encoding: identityin line with the spec, and arequest_builder_hookis provided for final customization.- The cancellation path via
CancelHandleandor_cancelwiring intoFetchRequestResourceis coherent.This all matches the expected semantics for Deno’s fetch implementation after the move.
573-635:op_fetch_send’s error propagation and response resource setup look correctThe logic in
op_fetch_sendto:
- Unwrap the single outstanding
FetchRequestResource(Rc::try_unwrap),- Distinguish between a canceled request and a genuine
FetchError,- Attempt to surface a nested error cause into
FetchResponse.errorfor JS reconstruction, and- Register a
FetchResponseResourcewith a correctcontent_lengthhintappears consistent and well‑structured. The behavior should closely mirror the previous
deno_fetchimplementation while adding better error introspection.ext/kv/Cargo.toml (1)
16-42: Dependency switch todeno_webverified as completeThe addition of
deno_web.workspace = truein ext/kv/Cargo.toml is appropriate and the migration is clean—no lingeringdeno_fetchreferences remain in the directory.runtime/js/99_main.js (1)
60-71: Import refactoring verified—fetch module correctly exports handleWasmStreamingThe new
ext:deno_web/26_fetch.jsproperly exportshandleWasmStreaming(line 608), the function definition exists (line 545), andruntime/js/99_main.jscorrectly uses it at line 400. All olddeno_fetchmodule imports have been removed.ext/websocket/02_websocketstream.js (1)
41-45: Headers helpers import verified; all required functions exportedVerified that
ext:deno_web/20_headers.jsexports all three required functions (fillHeaders,headerListFromHeaders,headersFromHeaderList) with unchanged signatures. The import refactoring in02_websocketstream.jsis correct.runtime/js/90_deno_ns.js (1)
11-13: HttpClient wiring migration verified and correctAll components of the migration from
deno_fetchtodeno_webare in place and functioning correctly:
ext/web/22_http_client.jsproperly exports bothcreateHttpClient(line 31) andHttpClientclass (line 103)- The import in
90_deno_ns.js(line 12) correctly references the new module path- Namespace wiring at lines 168–169 correctly exposes these symbols on the
Denoobject- No remnants of the old
deno_fetchpath remain in the runtimeext/node/Cargo.toml (1)
43-43: LGTM: Workspace dependency addition aligns with the migration.The addition of
deno_web.workspace = truecorrectly establishes the dependency needed for the deno_fetch to deno_web migration.ext/http/01_http.js (1)
34-45: LGTM: Import paths correctly updated to deno_web.The import sources for
InnerBody,ResponsePrototype,toInnerResponse,abortRequest,fromInnerRequest, andnewInnerRequesthave been consistently migrated fromext:deno_fetchtoext:deno_webwithout any changes to the imported symbols or logic.ext/node/lib.rs (1)
34-34: LGTM: Public re-export supports the migration.The addition of
pub use deno_web;expands the public API surface to include deno_web, which is consistent with the broader migration strategy and the dependency addition in Cargo.toml.ext/node/polyfills/http2.ts (1)
23-23: LGTM: Import path correctly migrated.The
toInnerRequestimport has been properly updated fromext:deno_fetch/23_request.jstoext:deno_web/23_request.js, maintaining consistency with the broader migration.ext/websocket/01_websocket.js (1)
74-75: LGTM: Header and HTTP client imports migrated correctly.Both import statements have been properly updated to use
ext:deno_webpaths while preserving all imported symbols (fillHeaders,headerListFromHeaders,headersFromHeaderList,HttpClientPrototype).ext/http/02_websocket.ts (1)
15-19: LGTM: Request and response imports migrated correctly.The import paths for
toInnerRequest,fromInnerResponse, andnewInnerResponsehave been consistently updated fromext:deno_fetchtoext:deno_webwithout any changes to the imported symbols.cli/tools/publish/mod.rs (1)
925-925: LGTM: ReqBody reference correctly updated.The module path has been properly updated from
deno_fetch::ReqBody::fulltodeno_web::fetch::ReqBody::full, reflecting the new module structure. The function call and its context remain unchanged.ext/web/23_response.js (1)
22-32: LGTM: Body and header imports migrated correctly.The import paths for
extractBody,mixinBody, and the header utilities (fillHeaders,getDecodeSplitHeader,guardFromHeaders,headerListFromHeaders,headersFromHeaderList) have been consistently updated to useext:deno_webpaths, completing the internal migration.ext/node/ops/http.rs (1)
44-46: Node HTTP now correctly reusesdeno_web::fetchtypes and helpersImports of
FetchCancelHandle,FetchReturn,ResBodyand the switch todeno_web::fetch::extract_authority/proxy::basic_authalign the Node HTTP client with the shared fetch implementation while preserving existing behavior and cancellation semantics. No issues spotted.Also applies to: 181-182, 241-245, 482-503
ext/http/00_serve.ts (1)
58-70: Serve internals correctly switched toext:deno_webrequest/response/body modulesThe updated imports for
InnerBody,fromInnerResponse/toInnerResponse, andabortRequestrouteDeno.servethrough the unifieddeno_webfetch implementation without changing behavior.ext/node/polyfills/http.ts (1)
72-74: Node HTTP polyfill now usesdeno_webheaders andResponsePointing
headersEntriesandResponseatext:deno_webkeeps the Node HTTP polyfill aligned with the consolidated fetch/web stack while preserving existing header and response handling semantics.ext/http/lib.rs (1)
142-146:deno_httpextension dependency list correctly dropsdeno_fetchSwitching the
depsarrays to[deno_web, deno_net, deno_websocket]matches the move of fetch functionality intodeno_weband keeps the HTTP extension’s JS modules (like00_serve.ts) correctly sourced.Also applies to: 193-196
ext/web/23_request.js (1)
31-43: Request module correctly switched toext:deno_webbody/headers/http-client helpersRouting
extractBody/mixinBody, header utilities, andHttpClientPrototypethroughext:deno_webkeepsRequestand the non-standardclientoption in sync with the consolidated fetch stack, without altering validation or cloning behavior.ext/web/lib.rs (1)
8-9:deno_webnow owns fetch (module, ops, JS assets, and options) coherentlyExporting the
fetchmodule, registering its ops, adding the corresponding JS modules (headers/body/request/response/fetch/eventsource), and threadingfetch_options: fetch::Optionsinto op state cleanly consolidates fetch underdeno_weband matches howext/web/fetchexpects to be configured. The wiring in this file looks consistent.Also applies to: 114-119, 142-150, 153-168
ext/kv/remote.rs (1)
78-79: FetchClient/FetchResponse migration todeno_web::fetchlooks consistentWrapping
deno_web::fetch::ClientandResBodyinFetchClient/FetchResponse, and switching todeno_web::fetch::ReqBody::fullinRemoteTransport::post, keeps the existingRemoteTransport/RemoteResponsesurface andBodyExt-based consumption semantics intact. Assumingdeno_web::fetch::ResBodycontinues to implement the samehttp_bodytraits (as it does inext/web/fetch), this should be a drop‑in replacement with no behavioral change.If you want an extra check, you can run the existing KV remote tests to confirm there’s no regression in streaming/collect behavior for responses.
Also applies to: 89-106
runtime/ops/web_worker/sync_fetch.rs (2)
10-11: Imports correctly updated todeno_web::fetchSwitching the imports to
deno_web::fetch::FetchErroranddeno_web::fetch::data_url::DataUrlmatches the new fetch module structure and keeps the rest of the file’s usages (FetchError::ClientCreate,DataUrl::process, etc.) coherent.Please confirm that
deno_web::fetch::FetchErrorstill exposes the same constructors/variants (ClientCreate,ClientSend,DataUrl,Base64,BlobNotFound) expected by this file.
98-101: Migration todeno_web::fetch::Optionsverified across all initialization and usage sitesThe verification confirms the migration is complete and correctly implemented:
- All references to the old
deno_fetch::Optionstype have been removed from the codebase- Both worker initialization paths (runtime/worker.rs and runtime/web_worker.rs) properly pass
deno_web::fetch::Optionsto the deno_web extension- The sync_fetch op at runtime/ops/web_worker/sync_fetch.rs:98 successfully borrows
deno_web::fetch::Optionsfrom state- The request body construction at lines 132-137 correctly uses
deno_web::fetch::ReqBody::empty()The architecture is sound: Options are passed to extension initialization functions, which handle storing them in OpState internally, allowing subsequent ops to borrow them as needed.
Signed-off-by: Asher Gomez <[email protected]>
|
PTAL @bartlomieju |
Done in the same spirit as #31198.
Unblocks #31205.