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

[dario-piotrowicz]

trafficstars

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

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 088bb5180475c6367de4f8101b9c032aff628f86

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]

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

Anyways the type of the request is still Request | string, so although this works fine typescript is not happy :cry: 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]

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?

[dario-piotrowicz]

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 :smiley:

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

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

[petebacondarwin]

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[bot]]

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

[codecov[bot]]

Codecov Report

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

:exclamation: 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%)	:arrow_up:
packages/wrangler/src/dev.tsx	84.65% <0.00%> (+3.36%)	:arrow_up:
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts	100.00% <0.00%> (+5.26%)	:arrow_up:
packages/wrangler/src/api/dev.ts	58.33% <0.00%> (+42.94%)	:arrow_up:

[mrbbot]

@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]

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

[petebacondarwin]

Yes please!

[dario-piotrowicz]

Yes please!

Done :slightly_smiling_face: https://github.com/cloudflare/workers-types/issues/277