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

fix: deprecate URL as the first arg of fetch #14113

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Mar 25, 2022

Deno's fetch and lib.dom's fetch have slightly different declarations. (Deno's fetch accept URL as the first arg, but lib.dom's fetch doesn't)

We should align Deno's declaration to lib.dom's (spec defines first arg as typedef (Request or USVString) RequestInfo ref) to reduce the confusion.

This PR tries to deprecate Deno-only signature of fetch function.

With this change, fetch with URL call gets strikethrough style in vscode like the below
スクリーンショット 2022-03-25 18 54 20

@kt3k kt3k merged commit 3462d87 into denoland:main Mar 25, 2022
@kt3k kt3k deleted the fix/deprecate-fetch-with-url-call branch March 25, 2022 11:23
@iugo
Copy link
Contributor

iugo commented Mar 26, 2022

This change does affect me. In some deno projects I've worked on, it does use a URL instead of a URL string as the first argument to fetch.

Perhaps, this PR should be an rf rather than a fix.

@kt3k
Copy link
Member Author

kt3k commented Mar 26, 2022

@iugo Does rf mean 'refactor'? BTW this doesn't change runtime. fetch(new URL(...)) still works. This just shows deprecation info in editors

@iugo
Copy link
Contributor

iugo commented Mar 26, 2022

@iugo Does rf mean 'refactor'? BTW this doesn't change runtime. fetch(new URL(...)) still works. This just shows deprecation info in editors

Although not a design refactoring of whatwg.fetch, this should be a design refactoring of deno ext/fetch.

Yes, just deprecate instead of remove helped us a lot. We'll adjust the code accordingly soon.

Also, to follow the whatwg standard more strictly, the first argument to fetch should start with " "about", "blob", "data", "file", "http" or "https". https://fetch.spec.whatwg.org/#url

@kt3k
Copy link
Member Author

kt3k commented Mar 26, 2022

Also, to follow the whatwg standard more strictly, the first argument to fetch should start with " "about", "blob", "data", "file", "http" or "https". https://fetch.spec.whatwg.org/#url

That might make sense, but probably we don't want to follow that in typing because we don't want to diverge our fetch typing from lib.dom's typing (which is maintained by TypeScript team)

Some applications use lib.dom instead of lib.deno.ns. If we have 2 different typings of fetch in those 2 type libs, that will cause the confusion in the ecosystem, which we'd like to avoid

@KnorpelSenf
Copy link
Contributor

This was a breaking change. My project does not compile anymore with 1.20.3 but it still does with 1.20.2. I understand that performing breaking changes in patch releases is attractive to do, but please somehow label this in your changelog next time.

Try

deno cache https://deno.land/x/grammy@v1.7.0/mod.ts

with the two versions to confirm.

@bebraw
Copy link

bebraw commented Mar 28, 2022

It looks like this broke things, yeah.

Consider the following:

> deno install -qAn vr https://deno.land/x/velociraptor@1.4.0/cli.ts        
error: TS2769 [ERROR]: No overload matches this call.
  Overload 1 of 2, '(input: string | Request, init?: RequestInit | undefined): Promise<Response>', gave the following error.
    Argument of type 'string | URL | Request' is not assignable to parameter of type 'string | Request'.
      Type 'URL' is not assignable to type 'string | Request'.  Overload 2 of 2, '(input: URL, init?: RequestInit | undefined): Promise<Response>', gave the following error.
    Argument of type 'string | URL | Request' is not assignable to parameter of type 'URL'.
      Type 'string' is not assignable to type 'URL'.
  const promise = fetch(url, { signal: controller.signal });
                        ~~~
    at https://arweave.net/e-arTRJlkythqLWvvPw09-2ZJU1rIS3l3asHysflhK0/lib/utilities/utils.ts:13:25

> deno --version                                                        
deno 1.20.3 (release, x86_64-apple-darwin)
v8 10.0.139.6
typescript 4.6.2

I was able to work around the issue by using the --no-check flag and I imagine that won't be needed when the dependencies catch up.

@nayeemrmn
Copy link
Collaborator

It will be fixed in the upcoming release

@kt3k
Copy link
Member Author

kt3k commented Mar 29, 2022

Oh, I didn't realize this change breaks those cases... Thank you for reporting! @KnorpelSenf @bebraw

@nayeemrmn Thank you for your fix!

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.

6 participants