-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
Document “AsyncGenerator” & “AsyncGeneratorFunction” #2861
Document “AsyncGenerator” & “AsyncGeneratorFunction” #2861
Conversation
Adds pages for the `AsyncGenerator` and `AsyncGeneratorFunction` standard built-in global objects that had been missing from the “Control abstraction objects” section. Fixes: #2803
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assigned the SVG team, so feel free to ignore comments you deem irrelevant (but it might be that others will bring them up again).
I focussed mainly on editorial bits here.
Not checked out and run locally.
files/en-us/web/javascript/reference/global_objects/asyncgenerator/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgenerator/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgenerator/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
Co-authored-by: André Jaenisch <[email protected]>
Co-authored-by: André Jaenisch <[email protected]>
Co-authored-by: André Jaenisch <[email protected]>
Thanks for all the help so far @Ryuno-Ki — it should be in much better shape now! Looking forward to seeing what the others think once they have a chance to take a look. |
Currently having a look at this - it looks great thank you @DerekNonGeneric Going to be a bit of time as I think BCD and member pages need to be looked at... will keep it updated in this conversation 👍
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look, a pending review (but only a small nitpick)
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
I'm pushing updates to this branch now - with any luck it'll work 😬 Once I've figured out the correct remotes... Awesome - looks like that worked - here are some things I've done:
Let me know what you think @Ryuno-Ki and @DerekNonGeneric The can of worms that has been opened because this needs thorough documenting are as follows (for my ref more than anything): Complete list:
Which would tidy up most of the dead links. I'm inclined to merge this and create a new issue/issues for these however. |
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
Thanks for the tidying up @Rumyra — it looks great! I like that you added an example that uses
If it would be easier for you to keep track of the whole can of worms in a single PR (i.e., to ensure no content is duplicated), we can handle those in this PR if you would prefer. Feel free to continue work on this branch. Arguably we should at least add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @DerekNonGeneric
Yes agreed - adding at least the methods to this PR would make it a nice rounded piece of work. We can do as previous and use the generator methods as a template Also temporarily converting to draft so it doesn't get merged just yet |
I got that taken care of, which brought to light that the variable names in one of the examples could use improvement. I think this is all we need to get this merged now:
|
@Rumyra Do you remember what this PR is waiting on? Glancing through the comments, it’s not clear to me at least… |
@sideshowbarker me :) The statement page still needs addressing - I'm just getting back to it now I'm back. Here's a more clear checklist:
|
Preview URLs
FlawsNote! 1 document with no flaws that don't need to be listed. 🎉 URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
External URLsURL: No new external URLs URL: No new external URLs URL: No new external URLs URL: No new external URLs URL: No new external URLs URL: No new external URLs URL: No new external URLs URL: No new external URLs URL: No new external URLs (this comment was updated 2021-07-14 16:08:45.690613) |
Soz I merged main into the latest commit and it's added a previous pr I made- I'll untangle after lunch 👍 |
This reverts commit c331788.
OK I think this is all ready to go - albeit being converted to markdown. Note I have removed the statement page as it's going to take a bit of work and was blocking this PR from going through. I will open an issue for it in https://github.com/mdn/content/projects/11 once we've got this going.
This could do all do with a good review @wbamberg @Elchi3 @sideshowbarker 🙏 |
About Markdown: it looks like there are 6 new files here: files/en-us/web/javascript/reference/global_objects/asyncgenerator/index.html ...and updates to 3 existing ones: files/en-us/web/javascript/reference/index.html For the new files, I reckon it makes sense to convert to Markdown for this PR and land them they way, whether or not the rest of the JS docs are converted at that point. I'll try converting them and see what breaks. For the existing files:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had quite a few comments, but some of them might be me not understanding this very well.
There seems to be some systematic problem with a jsxref reference - I noted most of them but probably missed some, so it's worth checking them all.
files/en-us/web/javascript/reference/global_objects/asyncgenerator/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgenerator/index.html
Outdated
Show resolved
Hide resolved
<dt>{{jsxref("AsyncGenerator.prototype.return()")}}</dt> | ||
<dd>Returns a {{jsxref("Promise")}} which will be resolved with the given value yielded by the {{jsxref("Operators/yield", "yield")}} expression and finishes the generator.</dd> | ||
<dt>{{jsxref("AsyncGenerator.prototype.throw()")}}</dt> | ||
<dd>Returns a {{jsxref("Promise")}} that is rejected with an exception thrown from (or uncaught from) within the async generator function and finishes the generator unless the exception is caught within that generator.</dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a long and complicated sentence and could do with splitting into 2, probably at the "function and finishes" juncture. It's also not very clear how to parse "(or uncaught from)" or how that relates to "unless the exception is caught within that generator".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talking about next and finishes make me think, whether a state chart might be helpful for comprehension.
files/en-us/web/javascript/reference/global_objects/asyncgenerator/next/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgenerator/next/index.html
Outdated
Show resolved
Hide resolved
|
||
<p>{{jsxref("Statements/async_function*", "async generator function")}} objects created with the <code>AsyncGeneratorFunction</code> constructor are parsed when the function is created. This is less efficient than declaring a generator function with an {{jsxref("Statements/async_function*", "async function* expression")}} and calling it within your code, because such functions are parsed with the rest of the code.</p> | ||
|
||
<p>All arguments passed to the function are treated as the names of the identifiers of the parameters in the function to be created, in the order in which they are passed.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the final argument?
files/en-us/web/javascript/reference/global_objects/asyncgeneratorfunction/index.html
Outdated
Show resolved
Hide resolved
|
||
<div class="notecard note"> | ||
<h3>Note:</h3> | ||
<p>{{jsxref("Statements/async_generator_function*", "async generator functions")}} created with the <code>AsyncGeneratorFunction</code> constructor do not create closures to their creation contexts; they are always created in the global scope.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again "async generator functions" should be "Async generator functions" if it's not an identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does global scope refers to here (in context of an ES module, for example. Would other modules see it, as in, it were defined on window
as property)?
|
||
<h2 id="Syntax">Syntax</h2> | ||
|
||
<pre class="brush: js">async function* [<var>name</var>]([<var>param1</var>[, <var>param2[</var>, ..., <var>paramN</var>]]]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it we are not using this form of syntax any more. Instead we are using separate lines for the variations (cf https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/forEach#syntax).
|
||
<h2 id="Description">Description</h2> | ||
|
||
<p>An <code>async function*</code> expression is very similar to and has almost the same syntax as a {{jsxref('Statements/async_function*', 'async function* statement', "", 1)}}. The main difference between an <code>async function*</code> expression and an <code>async function*</code> statement is the <em>function name</em>, which can be omitted in <code>async function*</code> expressions to create <em>anonymous</em> asynchronous generator functions. See also the chapter about {{jsxref("Functions", "functions")}} for more information.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, since "functions" is not an identifier this should be {{jsxref("Functions", "functions", "", "1")}}
I've tried converting this to Markdown and found a couple of things worth changing, but it might be simpler to apply them after content reviews are done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally not got a notification for #2861 (comment) (but got sent to commits instead which I shrugged away).
I looked at the documentation with an engineer's point of view. There are some inconsistencies which you might want to pick up or leave them as is. Your call :-)
files/en-us/web/javascript/reference/global_objects/asyncgenerator/index.html
Show resolved
Hide resolved
<dt>{{jsxref("AsyncGenerator.prototype.return()")}}</dt> | ||
<dd>Returns a {{jsxref("Promise")}} which will be resolved with the given value yielded by the {{jsxref("Operators/yield", "yield")}} expression and finishes the generator.</dd> | ||
<dt>{{jsxref("AsyncGenerator.prototype.throw()")}}</dt> | ||
<dd>Returns a {{jsxref("Promise")}} that is rejected with an exception thrown from (or uncaught from) within the async generator function and finishes the generator unless the exception is caught within that generator.</dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talking about next and finishes make me think, whether a state chart might be helpful for comprehension.
files/en-us/web/javascript/reference/global_objects/asyncgenerator/next/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgenerator/next/index.html
Outdated
Show resolved
Hide resolved
files/en-us/web/javascript/reference/global_objects/asyncgenerator/next/index.html
Outdated
Show resolved
Hide resolved
<dl> | ||
<dt><code><var>name</var></code> {{optional_inline}}</dt> | ||
<dd>The function name. Can be omitted, in which case the function is <em>anonymous</em>. The name is only local to the function body.</dd> | ||
<dt><code><var>paramN</var></code> {{optional_inline}}</dt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In another file, you wrapped N
in an em
element.
<dt><code><var>name</var></code> {{optional_inline}}</dt> | ||
<dd>The function name. Can be omitted, in which case the function is <em>anonymous</em>. The name is only local to the function body.</dd> | ||
<dt><code><var>paramN</var></code> {{optional_inline}}</dt> | ||
<dd>The name of an argument to be passed to the function. A function can have up to 255 arguments.</dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 255 (= 256-1 = 2^8-1)?
What happens if I exceed that number?
|
||
<h3 id="Using_function*">Using async function*</h3> | ||
|
||
<p>The following example defines an unnamed asynchronous generator function and assigns it to <code>x</code>. The function yields the square of its argument:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't „unnamed” imply „asynchronous”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x
is a bad variable name. square
might be better.
(I don't want to teach code newbies bad practices, so no offense).
yield await Promise.resolve(y * y) | ||
} | ||
|
||
x(6).next().then(res => console.log(res.value)); // logs 36 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're using another comment style here as elsewhere.
I can see conflicting files here. |
Thanks! I'm working my way through all these comments 👍
There should be - things have changed, that'll be the last thing I look at 👍 |
…atorfunction/index.html Co-authored-by: wbamberg <[email protected]>
…atorfunction/index.html Co-authored-by: wbamberg <[email protected]>
I’m not sure what this PR is waiting on currently, but it would be nice if we could get any remaining issues with it resolved and get it merged. In https://twitter.com/jcubic/status/1438938271761960968 someone who may be a potential contributor commented, “it has a lot of Open PR, the last open (on the last page) it's from March” (with an implication being that having PRs around that have been open for ~6 months might scare off some other potential contributors). |
It looks like some review comments are not addressed yet, and after review it all needs to be converted to Markdown. |
@wbamberg, I was under the impression that @Rumyra was going to wrap things up here, but if she doesn't have the bandwidth to get it over the finish line at the moment, I am happy to do so. I know there were a lot of other tasks associated w/ getting this PR ready to land, so I will see if I can coordinate via Matrix in the next couple days to see what can be done on my end. |
I have a working branch for this pr addressing the reviews. Once that's complete I intend to convert to markdown. So I'm closing this pr in favour of smaller, markdown esc prs for these pages 👍 |
--- | ||
<div>{{JSRef}}</div> | ||
|
||
<p>The <code><strong>AsyncGenerator</strong></code> object is returned by an {{jsxref("Statements/async_function*", "async generator function", "", 1)}} and it conforms to both the <a href="/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#The_iterable_protocol">iterable protocol</a> and the <a href="/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#The_iterator_protocol">iterator protocol</a>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the specification some more, I noticed that we may have made a copying error here when saying that it “conforms to both the iterable protocol and iterator protocol”, since it seems to actually conform to the async iterable protocol and async iterator protocol…
Refs: https://tc39.es/ecma262/#sec-asyncgenerator-objects
We should ensure that this is corrected in the PR that adds this page.
Unfortunately, it seems like MDN is missing pages for those protocols (previous links make no mention)…
/cc @Rumyra
Adds pages for the
AsyncGenerator
andAsyncGeneratorFunction
standard built-in global objects that had been missing from the
“Control abstraction objects” section.
Fixes: #2803
/cc @sideshowbarker @Ryuno-Ki @wbamberg