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(cli/web/fetch): Make Response constructor standard #5787

Merged
merged 11 commits into from
May 25, 2020

Conversation

marcosc90
Copy link
Contributor

@marcosc90 marcosc90 commented May 23, 2020

Fixes #4667 #4748

  • Remove duplicated Body implementation
  • Make constructor follow the spec (see notes regarding type)
  • res.body is now a ReadableStream

All tests are passing without modification, only removed a single test that was testing the previous Response constructor.


I'm not sure about response.type

  • How are basic, 'error' types set? In the previous implementation we weren't setting them either, so don't think this is a blocker right now.

Used a WeakMap to avoid adding extra settings to ResponseInit to be spec-compliant. I could use symbols instead, so please let me know.


Any comments or improvements are welcomed.

@marcosc90
Copy link
Contributor Author

marcosc90 commented May 23, 2020

Currently, this is most likely a breaking change. Although I'd like to modify Deno.copy to accept a ReadableStream too, so the following code will still work fine.

  const res = await fetch("http://example.com/file.bin");
  const file = await Deno.open("/tmp/file.bin", { create: true, write: true });

  await Deno.copy(res.body, file);
  file.close();

I'll submit the changes in a different PR.

@marcosc90 marcosc90 changed the title [WIP] fix(cli/web/fetch): Make Response constructor standard fix(cli/web/fetch): Make Response constructor standard May 23, 2020
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

LGTM

@ry
Copy link
Member

ry commented May 25, 2020

@marcosc90 could you please merge with master?

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @marcosc90

@ry ry merged commit 08f74e1 into denoland:master May 25, 2020
@marcosc90 marcosc90 deleted the fetch-response branch May 25, 2020 17:29
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.

Response constructor is non-standard
3 participants