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: Reword how-to section to explain how to use callbacks & controller #1614

Merged
merged 1 commit into from
May 11, 2023

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Mar 5, 2023

Instead of describing the callbacks one by one, putting emphasis on how/when the caller expects responses (upon completion, chunk-by-chunk, fire-and-forget).

This (hopefully) gives other specs who need to call into Fetch a quick reference between what they're trying to do and the API.

  • At least two implementers are interested (and none opposed):
    • N/A
  • Tests are written and can be reviewed and commented upon at:
    • N/A
  • Implementation bugs are filed:
    • N/A
  • MDN issue is filed: …
    • N/A

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@noamr
Copy link
Contributor Author

noamr commented Mar 5, 2023

/cc @jyasskin @domenic @annevk

Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! I think we should also run it by a couple of the more novice specification authors who have wondered how to integrate with Fetch recently, to see if it answers their questions. I'll send it to a couple once you've had a chance to integrate or disagree with my comments here.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Mar 8, 2023

This looks great, thanks! I think we should also run it by a couple of the more novice specification authors who have wondered how to integrate with Fetch recently, to see if it answers their questions. I'll send it to a couple once you've had a chance to integrate or disagree with my comments here.

Thanks! I accepted all your suggestions.

fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

A few final comments:

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Mar 15, 2023

Thanks @jyasskin !
@annevk @domenic care to review officially?

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
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.

Thanks for your work on this!

There's two outstanding comments from @jyasskin as well that need to be addressed, though note that one is slightly incorrect with respect to URLs being strings.

Generally I would not talk about "downloading" here as that's its own term of art within the context of navigation. Fetch fetches or obtains.

To set expectations, this needs at least one more close look after these issues are addressed.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Lots of editorial nits... please keep these sorts of things in mind for self-review in the future.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Mar 29, 2023

Lots of editorial nits... please keep these sorts of things in mind for self-review in the future.

Apologies, some of them I genuinely didn't know were mistakes, but a few I totally missed. Always trying to improve at this...

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented May 9, 2023

There are still many unresolved conversations above. Could you either resolve them or note what's needed to resolve them?

@noamr
Copy link
Contributor Author

noamr commented May 9, 2023

There are still many unresolved conversations above. Could you either resolve them or note what's needed to resolve them?

Done (I was looking at the file view so I missed those).

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.

@domenic it would be great if you could do another pass as well.

fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic 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 a few nits

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@noamr noamr closed this May 11, 2023
@noamr noamr reopened this May 11, 2023
Instead of describing the callbacks one by one, put emphasis on how/when the caller expects responses (upon completion, chunk-by-chunk, fire-and-forget).

This (hopefully) gives other specs who need to call into Fetch a quick reference between what they're trying to do and the API.

Co-Authored-By: Domenic Denicola <d@domenic.me>
Co-Authored-By: Anne van Kesteren <annevk@annevk.nl>
Co-Authored-By: Jeffrey Yasskin <jyasskin@gmail.com>
@annevk annevk merged commit 625897d into whatwg:main May 11, 2023
1 check passed
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.

None yet

6 participants