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

Editorial: add note on event listeners & preflight #578

Merged
merged 3 commits into from
Aug 17, 2017

Conversation

sideshowbarker
Copy link
Contributor

@sideshowbarker sideshowbarker commented Aug 12, 2017

We already have this in the XHR spec, but I think it would be helpful to
readers to also have a note in the Fetch spec at point of use, because the
Fetch spec is what gets cited at StackOverflow and such as the source for
requirements on what causes a preflight. So omitting mention of this case from
the Fetch spec risks making people (continue to) forget this is among the
things that could be causing a preflight they’re running into but that they
don’t understand the reason for or how to prevent it.


Preview | Diff

@annevk
Copy link
Member

annevk commented Aug 12, 2017

I think that and using a ReadableStream object for uploads are the sole reasons. We should probably call out both.

@sideshowbarker
Copy link
Contributor Author

I think that and using a ReadableStream object for uploads are the sole reasons. We should probably call out both.

OK, will add that. But where is the requirement defined about it happening when using a ReadableStream object for uploads?

I can’t seem to find anything either in the Fetch spec or Streams spec that states it…

@sideshowbarker
Copy link
Contributor Author

OK, will add that. But where is the requirement defined about it happening when using a ReadableStream object for uploads?

I can’t seem to find anything either in the Fetch spec or Streams spec that states it…

Discussed on IRC at http://logs.glob.uno/?c=freenode%23whatwg&s=12%20Aug%202017&e=12%20Aug%202017#c1034349

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

LGTM with lots of small nits.

fetch.bs Outdated
@@ -1003,6 +1003,11 @@ discouraged from using it for new features. It is rather unsafe. "<code>navigate
<dfn id=use-cors-preflight-flag export for=request>use-CORS-preflight flag</dfn>. Unless stated
otherwise, it is unset.

<p class="note no-backref">The <a>use-CORS-preflight flag</a> being set is just one of several
conditions that can result in a <a>CORS-preflight request</a>. The <a>use-CORS-preflight flag</a>
Copy link
Member

Choose a reason for hiding this comment

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

s/just//
s/can result/results/

fetch.bs Outdated
@@ -1003,6 +1003,11 @@ discouraged from using it for new features. It is rather unsafe. "<code>navigate
<dfn id=use-cors-preflight-flag export for=request>use-CORS-preflight flag</dfn>. Unless stated
otherwise, it is unset.

<p class="note no-backref">The <a>use-CORS-preflight flag</a> being set is just one of several
conditions that can result in a <a>CORS-preflight request</a>. The <a>use-CORS-preflight flag</a>
will be set if either one or more event listeners are registered on an {{XMLHttpRequestUpload}}
Copy link
Member

Choose a reason for hiding this comment

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

s/will be set/is set/

fetch.bs Outdated
<p class="note no-backref">The <a>use-CORS-preflight flag</a> being set is just one of several
conditions that can result in a <a>CORS-preflight request</a>. The <a>use-CORS-preflight flag</a>
will be set if either one or more event listeners are registered on an {{XMLHttpRequestUpload}}
object, or else if a {{ReadableStream}} object is used for uploads.
Copy link
Member

Choose a reason for hiding this comment

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

s/, or else/or if/ (I think that looks better)
s/for uploads/in a request/

@annevk annevk merged commit 8a91018 into master Aug 17, 2017
@annevk annevk deleted the sideshowbarker/event-listeners-and-preflight branch August 17, 2017 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants