fix: allow URL object to be passed to local `fetch()` calls #1769

dario-piotrowicz · 2022-09-04T14:17:23Z

Due to the local fetch check, URL objects are wrongly not accepted by fetch() calls,
amend the checkedFetch function in order to allow such scenario (which
anyways works after deployment).

resolves #1588

Due to the local fetch check, URL objects are wrongly not accepted by `fetch()` calls, amend the `checkedFetch` function in order to allow such scenario (which anyways works after deployment). resolves cloudflare#1588

changeset-bot · 2022-09-04T14:17:25Z

🦋 Changeset detected

Latest commit: 088bb51

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

dario-piotrowicz · 2022-09-04T15:08:36Z

Hi there 🙂 , I'm just trying to do this small bug fix, I hope my change makes sense 🙂

Anyways the type of the request is still Request | string, so although this works fine typescript is not happy 😢 I had a look into workers-types but if I understood correctly the fetch param types there are all auto-generated?
do the fetch param types need to be updated in the workers runtime? (PS: I had a look just to be sure and according to MDN fetch is indeed supposed to accept a URL object)

dario-piotrowicz · 2022-09-04T15:13:43Z

Also I couldn't fine unit tests regarding this fetch patching (that's why I didn't add any), is it not being tested? or am I just missing the tests?

petebacondarwin

Thanks for putting this together @dario-piotrowicz.
From looking at the MDN docs for fetch(), it says:

The fetch() method's parameters are identical to those of the Request() constructor.

So I would propose a simpler fix which is to always pass the fetch params to a Request() constructor and get the URL that way. E.g.

checkedFetch(request, init) {
  const url = new URL((new Request(request, init)).url;
  ...
}

WDYT?

And sorry that I never made any tests for this 😱 I don't know why...
It is a little awkward mocking out the real fetch() but we do it elsewhere.

dario-piotrowicz · 2022-09-05T21:13:04Z

Thanks for putting this together @dario-piotrowicz. From looking at the MDN docs for fetch(), it says:

The fetch() method's parameters are identical to those of the Request() constructor.

So I would propose a simpler fix which is to always pass the fetch params to a Request() constructor and get the URL that way. E.g.
checkedFetch(request, init) {
  const url = new URL((new Request(request, init)).url;
  ...
}
WDYT?

And sorry that I never made any tests for this scream I don't know why... It is a little awkward mocking out the real fetch() but we do it elsewhere.

Sounds good thanks 😃

About the tests if you want I can have a try in adding some 😄

by the way, what about the types then? how can I get the param types updated?

petebacondarwin · 2022-09-05T21:20:48Z

Regarding the typings, I think we probably could start by creating an issue here: https://github.com/cloudflare/workers-types.
From the docs, the request can be anything with a "stringifier" which doesn't seem very easy to define in pure TS.
@mrbbot could confirm but I think the fix would be in the Workers Runtime code internally at Cloudflare.

github-actions · 2022-09-05T21:20:49Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2996103266/npm-package-wrangler-1769

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1769/npm-package-wrangler-1769

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2996103266/npm-package-wrangler-1769 dev path/to/script.js

simplify solution

codecov · 2022-09-05T21:24:39Z

Codecov Report

Merging #1769 (634761b) into main (75f3ae8) will decrease coverage by 6.05%.
The diff coverage is n/a.

❗ Current head 634761b differs from pull request most recent head 088bb51. Consider uploading reports for the commit 088bb51 to get more accurate results

@@            Coverage Diff             @@
##             main    #1769      +/-   ##
==========================================
- Coverage   80.19%   74.13%   -6.06%     
==========================================
  Files          92       97       +5     
  Lines        6226     7068     +842     
  Branches     1603     1850     +247     
==========================================
+ Hits         4993     5240     +247     
- Misses       1233     1828     +595

Impacted Files	Coverage Δ
packages/wrangler/src/metrics/send-event.ts	`100.00% <0.00%> (ø)`
packages/wrangler/src/inspect.ts	`5.62% <0.00%> (ø)`
packages/wrangler/src/proxy.ts	`18.47% <0.00%> (ø)`
packages/wrangler/src/dev/local.tsx	`29.51% <0.00%> (ø)`
packages/wrangler/src/dev/start-server.ts	`69.49% <0.00%> (ø)`
packages/wrangler/src/dev-registry.tsx	`14.28% <0.00%> (ø)`
packages/wrangler/src/bundle.ts	`71.42% <0.00%> (+1.78%)`	⬆️
packages/wrangler/src/dev.tsx	`84.65% <0.00%> (+3.36%)`	⬆️
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts	`100.00% <0.00%> (+5.26%)`	⬆️
packages/wrangler/src/api/dev.ts	`58.33% <0.00%> (+42.94%)`	⬆️

mrbbot · 2022-09-05T21:35:17Z

@mrbbot could confirm but I think the fix would be in the Workers Runtime code internally at Cloudflare.

Yep, looks like the runtime code only accepts a Request and string, but I'm guessing there's some hidden magic to coerce values to strings. We could update the types generation script to account for this, but I think in this case the best solution would be to define an override for fetch accepting Request | string | URL.

dario-piotrowicz · 2022-09-05T22:07:47Z

I see, thanks @petebacondarwin and @mrbbot 🙂 , shall I then proceed and create an issue in https://github.com/cloudflare/workers-types?

petebacondarwin · 2022-09-06T07:18:31Z

Yes please!

dario-piotrowicz · 2022-09-06T08:22:48Z

Yes please!

Done 🙂
cloudflare/workers-types#277

add changeset

* fix: allow URL object to be passed to local `fetch()` calls Due to the local fetch check, URL objects are wrongly not accepted by `fetch()` calls, amend the `checkedFetch` function in order to allow such scenario (which anyways works after deployment). resolves #1588

GregBrimble · 2023-01-23T21:50:47Z

packages/wrangler/templates/checked-fetch.js

-	const url = new URL(
-		(typeof request === "string" ? new Request(request, init) : request).url
-	);
+	const url = new URL(new Request(request, init).url);


Ah, I think I got bitten by this change just the other day. Something to do with the request body being consumed twice. I'll look into it, but this should maybe be reverted.

Yes let's revert this. I can see this error using the reproduction in the linked issue.

#2562 (comment)

Here: #2597

sorry about the bug 😢 (thanks for the fix Pete 🙇)

I never thought to test it on POST requests coming out of a Worker.
The reproduction required a fetch() call to a Request that has a body (but GET requests cannot have bodies).

This reverts and fixes the changes in cloudflare#1769 which does not support creating requests from requests whose bodies have already been consumed. Fixes cloudflare#2562

…tch (#2597) This reverts and fixes the changes in #1769 which does not support creating requests from requests whose bodies have already been consumed. Fixes #2562

fix: allow URL object to be passed to local fetch() calls

6f52bc9

Due to the local fetch check, URL objects are wrongly not accepted by `fetch()` calls, amend the `checkedFetch` function in order to allow such scenario (which anyways works after deployment). resolves cloudflare#1588

dario-piotrowicz requested review from threepointone and petebacondarwin as code owners September 4, 2022 14:17

petebacondarwin requested changes Sep 5, 2022

View reviewed changes

dario-piotrowicz requested review from petebacondarwin and removed request for threepointone September 5, 2022 21:19

fixup! fix: allow URL object to be passed to local fetch() calls

634761b

simplify solution

dario-piotrowicz force-pushed the 1588 branch from 2179852 to 634761b Compare September 5, 2022 21:24

dario-piotrowicz mentioned this pull request Sep 6, 2022

The fetch() request parameter's type is not correct cloudflare/workers-types#277

Closed

fixup! fix: allow URL object to be passed to local fetch() calls

088bb51

add changeset

petebacondarwin approved these changes Jan 12, 2023

View reviewed changes

petebacondarwin merged commit 6a67efe into cloudflare:main Jan 12, 2023

github-actions bot mentioned this pull request Jan 12, 2023

Version Packages #2531

Merged

carlsonorozco mentioned this pull request Jan 18, 2023

🐛 BUG: Cannot construct a Request with a Request object that has already been used #2562

Closed

GregBrimble reviewed Jan 23, 2023

View reviewed changes

petebacondarwin mentioned this pull request Jan 23, 2023

fix: do not crash in wrangler dev when passing a request object to fetch #2597

Merged

6 tasks

dario-piotrowicz deleted the 1588 branch January 23, 2023 22:46

github-actions bot mentioned this pull request Jan 23, 2023

Version Packages #2595

Merged

renovate bot mentioned this pull request Feb 23, 2023

chore(deps): update all non-major dependencies aynh/cf-anime-quotes#5

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow URL object to be passed to local `fetch()` calls #1769

fix: allow URL object to be passed to local `fetch()` calls #1769

dario-piotrowicz commented Sep 4, 2022

changeset-bot bot commented Sep 4, 2022 •

edited

Loading

dario-piotrowicz commented Sep 4, 2022

dario-piotrowicz commented Sep 4, 2022

petebacondarwin left a comment

dario-piotrowicz commented Sep 5, 2022

petebacondarwin commented Sep 5, 2022

github-actions bot commented Sep 5, 2022 •

edited

Loading

codecov bot commented Sep 5, 2022 •

edited

Loading

mrbbot commented Sep 5, 2022

dario-piotrowicz commented Sep 5, 2022

petebacondarwin commented Sep 6, 2022

dario-piotrowicz commented Sep 6, 2022

GregBrimble Jan 23, 2023

petebacondarwin Jan 23, 2023

petebacondarwin Jan 23, 2023

dario-piotrowicz Jan 23, 2023

petebacondarwin Jan 23, 2023

fix: allow URL object to be passed to local fetch() calls #1769

fix: allow URL object to be passed to local fetch() calls #1769

Conversation

dario-piotrowicz commented Sep 4, 2022

changeset-bot bot commented Sep 4, 2022 • edited Loading

🦋 Changeset detected

dario-piotrowicz commented Sep 4, 2022

dario-piotrowicz commented Sep 4, 2022

petebacondarwin left a comment

Choose a reason for hiding this comment

dario-piotrowicz commented Sep 5, 2022

petebacondarwin commented Sep 5, 2022

github-actions bot commented Sep 5, 2022 • edited Loading

codecov bot commented Sep 5, 2022 • edited Loading

Codecov Report

mrbbot commented Sep 5, 2022

dario-piotrowicz commented Sep 5, 2022

petebacondarwin commented Sep 6, 2022

dario-piotrowicz commented Sep 6, 2022

GregBrimble Jan 23, 2023

Choose a reason for hiding this comment

petebacondarwin Jan 23, 2023

Choose a reason for hiding this comment

petebacondarwin Jan 23, 2023

Choose a reason for hiding this comment

dario-piotrowicz Jan 23, 2023

Choose a reason for hiding this comment

petebacondarwin Jan 23, 2023

Choose a reason for hiding this comment

fix: allow URL object to be passed to local `fetch()` calls #1769

fix: allow URL object to be passed to local `fetch()` calls #1769

changeset-bot bot commented Sep 4, 2022 •

edited

Loading

github-actions bot commented Sep 5, 2022 •

edited

Loading

codecov bot commented Sep 5, 2022 •

edited

Loading