Skip to content

Implement Request.clone#92

Merged
guybedford merged 5 commits intomainfrom
request-clone
Jul 30, 2024
Merged

Implement Request.clone#92
guybedford merged 5 commits intomainfrom
request-clone

Conversation

@tschneidereit
Copy link
Copy Markdown
Member

This is fairly straightforward by itself. I threw in a couple of additional commits that clean a few things up. They come with detailed commit messages describing what they're about. Let me know if you'd prefer splitting them off, though.

Fixes #84

It already is infallible internally, so might as well reflect that in the signature and clean up the callsites.
I stumbled upon this while implementing `Request.clone`, and while this change doesn't cause any additional WPT tests to pass, I'm certain that it's required: otherwise, `new Request(incomingRequest)` will not apply `incomingRequest`'s header entries to the new request if `.headers` has never been read on `incomingRequest`.
@tschneidereit tschneidereit requested a review from guybedford July 27, 2024 17:09
Copy link
Copy Markdown
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks great. Since there isn't WPT coverage for this, can we add an integration test? For example, porting the cases from https://github.com/fastly/js-compute-runtime/blob/main/integration-tests/js-compute/fixtures/app/src/request-clone.js.

@JakeChampion
Copy link
Copy Markdown
Contributor

Looks great. Since there isn't WPT coverage for this, can we add an integration test? For example, porting the cases from https://github.com/fastly/js-compute-runtime/blob/main/integration-tests/js-compute/fixtures/app/src/request-clone.js.

Perhaps we need to update wpt?
https://wpt.fyi/results/fetch/api/request/request-clone.sub.html?label=experimental&label=master&aligned

@guybedford
Copy link
Copy Markdown
Contributor

@JakeChampion we are on a recent WPT, we just don't run html tests.

@guybedford
Copy link
Copy Markdown
Contributor

guybedford commented Jul 30, 2024

@tschneidereit if it helps, I added a simple clone test here, which is giving an error just on this line:

deepStrictEqual([...newRequest.headers], [...request.headers], 'newRequest.headers');

with the stdout when that line is enabled (from tests/integration/fetch/stderr.log) reading:

stderr [0] :: Warning: Using a DEBUG build. Expect things to be SLOW.
stderr [0] :: "POST"
stderr [0] :: [-1] Assertion failure: isObjectOrNull(), at 
stderr [0] :: /Users/gbedford/Projects/StarlingMonkey/deps/cpm_cache/spidermonkey-debug/76821ccb5a765a81c18400e00f9c7207f1fd9461/spidermonkey-debug/include/js/Value.h
stderr [0] :: :998
2024-07-30T00:10:41.598639Z ERROR wasmtime_cli::commands::serve: [0] :: Error {
    context: "error while executing at wasm backtrace:\n    0: 0xdf6f9 - <unknown>!JS::Value::toObjectOrNull() const\n    1: 0x10f6aa - <unknown>!builtins::web::fetch::RequestOrResponse::maybe_headers(JSObject*)\n    2: 0x10f214 - <unknown>!builtins::web::fetch::RequestOrResponse::headers(JSContext*, JS::Handle<JSObject*>)\n    3: 0x119806 - <unknown>!builtins::web::fetch::Request::headers_get(JSContext*, unsigned int, JS::Value*)\n    4: 0x2565eb - <unknown>!CallJSNative(JSContext*, bool (*)(JSContext*, un

@guybedford
Copy link
Copy Markdown
Contributor

I went ahead and pushed up a fix for the headers cloning bug, will land this later today.

@tschneidereit
Copy link
Copy Markdown
Member Author

Thank you for doing that! <3

Can you say what the fix actually was? That commit for me renders with lots of changes that seem to only change whitespace (even if I set the "ignore whitespace changes" flag) so it's hard to tell what actually changed

@tschneidereit
Copy link
Copy Markdown
Member Author

Oh, never mind: clicking on the commit in the email notification showed something entirely different from what I get when viewing only this commit in the PR itself 🤷🏻

@guybedford
Copy link
Copy Markdown
Contributor

Yes, I applied clang format by mistake, then pushed up the correction.

@tschneidereit
Copy link
Copy Markdown
Member Author

Ah, that makes sense!

As does the fix :)

@guybedford guybedford merged commit 1a1dc3d into main Jul 30, 2024
@guybedford guybedford deleted the request-clone branch July 30, 2024 17:51
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.

Request.clone() regression

3 participants