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

[WIP] Make fetch() more standards compliant #3667

Merged
merged 33 commits into from
Feb 3, 2020

Conversation

serverhiccups
Copy link
Contributor

@serverhiccups serverhiccups commented Jan 13, 2020

Hi all,
This is my first time contributing to this repo (and using typescript), so I'm very open to comments regarding code style/cleanliness.

Currently fetch returns notImplemented on many options, so I'm just trying to plug those gaps.
The relevant spec is here: https://fetch.spec.whatwg.org/.
Fixes #3660.

To-do list (in no particular order):

  • Support different redirect modes (including non-standard nofollow)
  • Support Reponse.redirect()
  • Allow multiple response types (eg. other than "basic") (And by extension, NetworkError)
  • Add tests for everything

For later:
- [ ] Refactor the Response object so that we can handle 'internal' headers See #3767

@claassistantio
Copy link

claassistantio commented Jan 13, 2020

CLA assistant check
All committers have signed the CLA.

@serverhiccups serverhiccups changed the title Make fetch() more standards compliant [WIP] Make fetch() more standards compliant Jan 13, 2020
@serverhiccups
Copy link
Contributor Author

Oh dear, It seems like the linting is failing, which is weird given that it works on my laptop. 🤔

@kt3k
Copy link
Member

kt3k commented Jan 13, 2020

@serverhiccups lint CI status includes the checking of formatting of source code. Try run ./tools/format.py --rs.

@serverhiccups
Copy link
Contributor Author

RFC on whether or not we should add a new "nofollow" redirect mode. I'm suggesting this because I'm currently working on a project that requires the ability to view the response of a request that always returns 302. While I know this PR is supposed to be about making fetch more standards-compliant, but it seems like a bad idea to remove a really useful piece of functionality to make fetch exactly by the book.

@serverhiccups serverhiccups force-pushed the make-fetch-more-compliant branch from 1c093a8 to e0d6bb0 Compare January 14, 2020 02:16
@serverhiccups
Copy link
Contributor Author

serverhiccups commented Jan 14, 2020

I think that moving the refactoring of the Response object (to properly support the standard and 'internal` responses) to separate issue is a good idea because it would probably significantly delay the rest of this PR. That being said, because Deno doesn't really need to prevent reading the internal states of responses like browser (because we aren't trying to prevent cross-origin requests), our currently solution is probably fine.

@ry
Copy link
Member

ry commented Jan 14, 2020

@serverhiccups This PR is looking good - thanks! Looking forward to some tests.

RFC on whether or not we should add a new "nofollow" redirect mode.

Can you explain what nofollow does? I only know of the rel=nofollow which relates to how search engines treat the link.

@serverhiccups
Copy link
Contributor Author

@ry I probably should have explained it better. nofollow is just a name I came up with when writing the code, as I was unaware of rel=nofollow. What it does is return the actual response even when a server provides a 30x status, as opposed to manual which returns an opaqueredirect Response that includes basically no information (see here).
I'm not attached to the name nofollow, but I couldn't think of a name that conveyed the meaning better (that wasn't manual, which would be perfect). Perhaps onetime or something similar would be better.

@serverhiccups
Copy link
Contributor Author

Perf test is failing now? I'm not sure if that is related to my code...

@serverhiccups serverhiccups force-pushed the make-fetch-more-compliant branch from 0cca597 to 3cef47a Compare January 16, 2020 08:31
@ry
Copy link
Member

ry commented Jan 16, 2020

@serverhiccups I didn't know about these redirect modes. So you need to get the headers for a 3xx response? That seems like a reasonable thing to do... Is it impossible in browsers?

I'm hesitant to add non-standard extensions to fetch. Maybe a way forward would be to split this PR into two parts - one where the standard request modes are handled, and then a second which adds a new request mode ("nofollow" is confusing I think, maybe "noredirect"?)

@bartlomieju
Copy link
Member

Redirection behaviour can be controlled by Request using init.redirect option.

https://developer.mozilla.org/en-US/docs/Web/API/Request/Request

@serverhiccups
Copy link
Contributor Author

@ry AFAIK as I know, it is technically possible to not be redirected in browsers. However, both ways of doing it (redirect: manual and redirect: error) return response objects with no useful information (SO answer and standard). I do understand not wanting to add something non-standard to fetch, but I couldn't think of another way of getting the behaviour that I want. It would seem a shame to leave that functionality out of deno. I can definitely move 'noredirect' into a different PR so that we can get the other things merged in sooner.

@bartlomieju While you can control the redirection mode, you can't actually control it in meaningful ways because the modes that don't redirect don't provide any useful information in the response (See here).

I'm going to (hopefully) finish the tests for this PR today and then I'll split the noredirect functionality into a separate PR so that we can get everything else merged.

@bartlomieju
Copy link
Member

@serverhiccups is this PR ready for review?

@serverhiccups serverhiccups force-pushed the make-fetch-more-compliant branch from f737a75 to 360e2fb Compare January 21, 2020 21:35
@serverhiccups
Copy link
Contributor Author

Now the build is failing :(. Not sure why, doesn't seem to be related to my changes...

cli/js/fetch.ts Outdated Show resolved Hide resolved
@serverhiccups
Copy link
Contributor Author

I'm pretty aware that the commit history on this one is pretty ugly, but I'm not entirely sure how to fix it, and I'm just happy it works.

Also clarify some checks in Response.
@serverhiccups
Copy link
Contributor Author

I'm pretty sure this is commit is about ready to land. I'm going to create a separate PR for the noredirect redirect mode and an issue for implementing 'internal responses`.

@serverhiccups serverhiccups marked this pull request as ready for review January 26, 2020 06:16
@serverhiccups serverhiccups requested a review from ry February 1, 2020 22:59
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 - thank you @serverhiccups - sorry for the delay

@bartlomieju bartlomieju merged commit 2b0cf74 into denoland:master Feb 3, 2020
@serverhiccups serverhiccups deleted the make-fetch-more-compliant branch February 7, 2020 22:14
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.redirect is missing
6 participants