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

Support credentials #56

Merged
merged 4 commits into from
Jan 11, 2015
Merged

Support credentials #56

merged 4 commits into from
Jan 11, 2015

Conversation

josh
Copy link
Contributor

@josh josh commented Jan 6, 2015

Incomplete.

I'm not sure how to support credentials: 'omit' with XHR.

Closes #43.

/cc @dgraham @annevk @mislav

@@ -196,3 +196,43 @@ promiseTest('supports HTTP DELETE', 2, function() {
equal(request.data, '')
})
})

promiseTest('doesnt send cookies with implicit omit credentials', 1, function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

"does not"

@dgraham
Copy link
Contributor

dgraham commented Jan 6, 2015

We need to default to "omit" in the Request constructor.

I wish the default was same-origin rather than omit. All of our fetch usage requires cookie headers.

@josh
Copy link
Contributor Author

josh commented Jan 6, 2015

Any ideas how to disable cookies for 'omit'? It seems like withCredentials is always ignored for same origin requests.

@annevk
Copy link

annevk commented Jan 6, 2015

That feature does not exist in XMLHttpRequest. XMLHttpRequest only has same-origin and CORS.

I could be convinced to have the standard default to same-origin, but it seems rather ugly to have a different default from cross-origin. (Mistake with how we designed XMLHttpRequest...)

@josh
Copy link
Contributor Author

josh commented Jan 6, 2015

@annevk it seems like the majority of fetch contexts (image, script, style, subresources) mostly use "include" even for cross origin resources with the exception of XHR's CORS semantics. I kinda agree that world would be a better place if stuff like script was actually omit by default. But for most direct fetch() api calls, I think I'll be using same-origin most the time.

@annevk
Copy link

annevk commented Jan 7, 2015

Well they use include but also set mode to no-cors by default. If you use the crossorigin attribute I think the intent is omit and mode cors, though implementations might do same-origin and mode cors...

@dgraham
Copy link
Contributor

dgraham commented Jan 7, 2015

I could be convinced to have the standard default to same-origin

@annevk From my perspective, a default of same-origin is most useful. A default of omit means that fetch cannot be used easily by sites with user sessions maintained by cookie headers.

A simple fetch('/some/url') call becomes fetch('/some/url', {credentials: 'same-origin'}) at each call site. I'd prefer to specify omit in the few cases it's needed instead.

For now, we've added a default of same-origin to the $.fetch wrapper we use on github.com.

If you think it's appropriate, maybe the default in the specification could be changed to same-origin?

@annevk
Copy link

annevk commented Jan 8, 2015

I recommend filing a bug against the specification. I don't really like same-origin myself as what it does depends on the origin of the URL you passed in, which is super weird.

josh added a commit that referenced this pull request Jan 11, 2015
@josh josh merged commit 6617c40 into master Jan 11, 2015
@josh josh deleted the test-cookies branch January 11, 2015 03:15
xg-wang pushed a commit to xg-wang/pretender that referenced this pull request May 23, 2018
tl;dr
- This commit add support for fetch.
- pretender swap native fetch related API if exists
- pretender.shutdown() restore native fetch related API
- doesn't work with AbortController

Pretender has been working well with xhr, but doesn't handle fetch.
It's been 2 years since @rwjblue first open the
[issue](pretenderjs#60) for fetch
support. So people don't need to do extra work to polyfill for testing.

Include a fetch ponyfill and swap the native fetch during pretender
creation, then restore them when `shutdown`. Since fetch polyfill uses
xhr behind the scene, pretender will "just work".

Per Yetch homepage, the supplement set of yetch impl and spec include:
- Inability to [set the redirect mode](JakeChampion/fetch#137)
- Inability to [change the cache directive](JakeChampion/fetch#438 (comment))
- Inability to [disable same-origin cookies](JakeChampion/fetch#56 (comment))

- `xhr.abort()` first set state to done, finally response to a [network error](https://xhr.spec.whatwg.org/#the-abort()-method);
- [fetch](https://dom.spec.whatwg.org/#aborting-ongoing-activities) will reject promise with a new "AbortError" DOMException.

For `fake_xml_http_request` impl, the request is resolved once its state is changed to `DONE` so the `reject` is not cathed.

So the senario happens in pretender is:
1. state chagne to `DONE`, trigger resolve request
2. abort, trigger reject
3. xhr.onerror, trigger reject

The first resolve wins, error thus not rejected but an empty request is resolved.

Though polyfilled by xhr, fetch returns a Promise and is asynchronous by
nature.
xg-wang pushed a commit to xg-wang/pretender that referenced this pull request May 23, 2018
tl;dr
- This commit add support for fetch.
- pretender swap native fetch related API if exists
- pretender.shutdown() restore native fetch related API
- doesn't work with AbortController

Motivation
------
Pretender has been working well with xhr, but doesn't handle fetch.
It's been 2 years since @rwjblue first open the
[issue](pretenderjs#60) for fetch
support. So people don't need to do extra work to polyfill for testing.

Changes
------
Include a fetch ponyfill and swap the native fetch during pretender
creation, then restore them when `shutdown`. Since fetch polyfill uses
xhr behind the scene, pretender should "just work".

Caveats
------
1. The supplement set of yetch impl and spec includes (not complete):
- Inability to [set the redirect mode](JakeChampion/fetch#137)
- Inability to [change the cache directive](JakeChampion/fetch#438 (comment))
- Inability to [disable same-origin cookies](JakeChampion/fetch#56 (comment))

2. Abort
- `xhr.abort()` first set state to done, finally response to a [network error](https://xhr.spec.whatwg.org/#the-abort()-method);
- [fetch](https://dom.spec.whatwg.org/#aborting-ongoing-activities) will reject promise with a new "AbortError" DOMException.

As implemented in `fake_xml_http_request`, the request is resolved once its
state is changed to `DONE`.
So the senario happens in pretender is:
  1). state chagne to `DONE`, trigger resolve request
  2). abort, trigger reject
  3). xhr.onerror, trigger reject
The first resolve wins, error thus not rejected but an empty request is resolved.

3. Though polyfilled by xhr, fetch returns a Promise and is asynchronous by
nature.
xg-wang pushed a commit to xg-wang/pretender that referenced this pull request May 23, 2018
tl;dr
- This commit adds support for fetch.
- pretender swap native fetch related API if exists
- pretender.shutdown() restore native fetch related API
- doesn't work with AbortController

Motivation
------
Pretender has been working well with xhr, but doesn't handle fetch.
It's been 2 years since @rwjblue first open the
[issue](pretenderjs#60) for fetch
support. So people don't need to do extra work to polyfill for testing.

Changes
------
Include a fetch ponyfill and swap the native fetch during pretender
creation, then restore them when `shutdown`. Since fetch polyfill uses
xhr behind the scene, pretender should "just work".

Caveats
------
1. The supplement set of yetch impl and spec includes (not complete):
- Inability to [set the redirect mode](JakeChampion/fetch#137)
- Inability to [change the cache directive](JakeChampion/fetch#438 (comment))
- Inability to [disable same-origin cookies](JakeChampion/fetch#56 (comment))

2. Abort
- `xhr.abort()` first set state to done, finally response to a
[network error](https://xhr.spec.whatwg.org/#the-abort()-method);
- [fetch](https://dom.spec.whatwg.org/#aborting-ongoing-activities) will
reject promise with a new "AbortError" DOMException.

As implemented in `fake_xml_http_request`, the request is resolved once its
state is changed to `DONE`.
So the senario happens in pretender is:
  1). state changes to `DONE`, trigger resolve request
  2). abort, trigger reject
  3). xhr.onerror, trigger reject
The first resolve wins, error thus not rejected but an empty request is resolved.

3. Though polyfilled by xhr, fetch returns a Promise and is asynchronous by
nature.
xg-wang pushed a commit to xg-wang/pretender that referenced this pull request May 23, 2018
tl;dr
- This commit adds support for fetch, fix pretenderjs#60.
- pretender swap native fetch related API if exists
- pretender.shutdown() restore native fetch related API
- doesn't work with AbortController

Changes
------
Include a fetch ponyfill and swap the native fetch during pretender
creation, then restore them when `shutdown`. Since fetch polyfill uses
xhr behind the scene, pretender should "just work".

Caveats
------
1. The supplement set of yetch impl and spec includes (not complete):
- Inability to [set the redirect mode](JakeChampion/fetch#137)
- Inability to [change the cache directive](JakeChampion/fetch#438 (comment))
- Inability to [disable same-origin cookies](JakeChampion/fetch#56 (comment))

2. Abort
- `xhr.abort()` first set state to done, finally response to a
[network error](https://xhr.spec.whatwg.org/#the-abort()-method);
- [fetch](https://dom.spec.whatwg.org/#aborting-ongoing-activities) will
reject promise with a new "AbortError" DOMException.

As implemented in `fake_xml_http_request`, the request is resolved once its
state is changed to `DONE`.
So the scenario happens in pretender is:
  1). state changes to `DONE`, trigger resolve request
  2). abort, trigger reject
  3). xhr.onerror, trigger reject
The first resolve wins, error thus not rejected but an empty request is resolved.

3. Though polyfilled by xhr, fetch returns a Promise and is asynchronous by
nature.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2020
ppichate2531 referenced this pull request in ppichate2531/css Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test for HTTP cookies
4 participants