Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Add code example #150

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add code example #150

wants to merge 2 commits into from

Conversation

mathiasbynens
Copy link
Member

@mathiasbynens mathiasbynens commented Jul 9, 2019

WDYT? cc @sigurdschneider

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

I like the intent. With the removal of non-EcmaScript jargon like "microtask checkpoint" this would LGTM. Thanks!

README.md Outdated
}
```

The guarantee goes away if any of the blanks (`[…]`) introduces a microtask checkpoint, for example if it contains an `await` expression, or if it wraps the remainder of the code in a promise callback (e.g. `Promise.resolve().then(() => […])`).
Copy link
Contributor

Choose a reason for hiding this comment

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

microtask checkpoint

This is not an EcmaScript concept.

or if it wraps the remainder of the code in a promise callback (e.g. Promise.resolve().then(() => […]))

probably worth saying somehow. But literally this doesn't make syntactic sense for the example code, as it is shown as straight-line code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is re-using the same "microtask checkpoint" terminology from the previous paragraph. It's indeed HTML spec jargon. I'm definitely open to rephrasing it to make it more general; could you recommend a better phrasing?

But literally this doesn't make syntactic sense for the example code, as it is shown as straight-line code.

It only makes sense if you think of each // […] as an injection point where arbitrary text can go. One could then replace the first // […] with:

Promise.resolve().then(() => {

...and the last // […] with:

});

Any idea on how to make this more clear? Or should we just drop this particular example?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is re-using the same "microtask checkpoint" terminology from the previous paragraph. It's indeed HTML spec jargon. I'm definitely open to rephrasing it to make it more general; could you recommend a better phrasing?

Thanks for pointing out the previous paragraph. That language needs to go too. EcmaScript has no tasks or microtasks. As of the last tc39 mtg, @littledan and I arrived at a mutual understanding about how to phrase all of this in terms of Job scheduling and Job execution. I'll first let @littledan speak for his memory of our mutual understanding and how it bears on this issue before making my own suggestions.

@littledan , did we record our mutual understanding? Was it in your subsequent talk slides? Meeting notes? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea on how to make this more clear? Or should we just drop this particular example?

I see what you're doing and it is coherent and informative. However, you're filling in the [...]s with unbalanced bracketing, which would change the indentation level. That's what's confusing. Although not an official part of the syntax, we all understand code in terms of indentation level.

Yes, I think the best thing is probably just to drop this particular example, assuming everyone understands the shown indentation level to rule it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out the previous paragraph. That language needs to go too.

Did you see the previous paragraph? It explicitly points out that this is an HTML concept, which seems fine by me.

Either way, I'd love to hear more about the discussion you had with Dan. He's on vacation now so I imagine it'll take a while until we hear back.

I see what you're doing and it is coherent and informative. However, you're filling in the [...]s with unbalanced bracketing, which would change the indentation level. That's what's confusing. Although not an official part of the syntax, we all understand code in terms of indentation level.

Yes, I think the best thing is probably just to drop this particular example, assuming everyone understands the shown indentation level to rule it out.

Ack. Removed this example.

README.md Outdated
}
```

The guarantee goes away if any of the blanks (`[…]`) introduces a microtask checkpoint, for example if it contains an `await` expression, or if it wraps the remainder of the code in a promise callback (e.g. `Promise.resolve().then(() => […])`).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is re-using the same "microtask checkpoint" terminology from the previous paragraph. It's indeed HTML spec jargon. I'm definitely open to rephrasing it to make it more general; could you recommend a better phrasing?

Thanks for pointing out the previous paragraph. That language needs to go too. EcmaScript has no tasks or microtasks. As of the last tc39 mtg, @littledan and I arrived at a mutual understanding about how to phrase all of this in terms of Job scheduling and Job execution. I'll first let @littledan speak for his memory of our mutual understanding and how it bears on this issue before making my own suggestions.

@littledan , did we record our mutual understanding? Was it in your subsequent talk slides? Meeting notes? Thanks.

README.md Outdated
}
```

The guarantee goes away if any of the blanks (`[…]`) introduces a microtask checkpoint, for example if it contains an `await` expression, or if it wraps the remainder of the code in a promise callback (e.g. `Promise.resolve().then(() => […])`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea on how to make this more clear? Or should we just drop this particular example?

I see what you're doing and it is coherent and informative. However, you're filling in the [...]s with unbalanced bracketing, which would change the indentation level. That's what's confusing. Although not an official part of the syntax, we all understand code in terms of indentation level.

Yes, I think the best thing is probably just to drop this particular example, assuming everyone understands the shown indentation level to rule it out.

@littledan
Copy link
Member

Note, the deref stability is still guaranteed as long as you don't return to the event loop. So await Promise.resolve() will still preserve stability, for example. This error should be fixed before the PR is merged.

@mhofman
Copy link
Member

mhofman commented Sep 4, 2019

Note, the deref stability is still guaranteed as long as you don't return to the event loop. So await Promise.resolve() will still preserve stability, for example. This error should be fixed before the PR is merged.

Actually there currently is no longer such a guarantee in the spec text. The only constraint on the scheduling of the ClearKeptObjects operation is that it doesn't interrupt synchronous execution.

It is actually causing some confusion on testability in tc39/test262#2239 (comment)

@littledan
Copy link
Member

It's guaranteed on the web, given the form of the HTML integration. The current text implies that there always may be a collection when there is an await (a frequently repeated misconception), when embedding environments have the freedom to determine whether that's the case (and I would encourage others environments to align with the web here, for the same reasons we made the decision in that case).

@mhofman
Copy link
Member

mhofman commented Sep 5, 2019

TBH, I still don't understand the reasoning for the timing guarantees of ClearKeptObjects.

What is the advantage of guaranteeing await Promise.resolve() will preserve stability in HTML?
As a developer, the mental model of await asyncResult is that the function will continue execution at a later time. As such I would not expect wr.deref() to be stable across any await. I consider it to be an optimization / internal detail that execution continues without returning to the run-loop if asyncResult is a resolved promise.

Wouldn't it be simpler to say that wr.deref() is stable in a synchronous execution, but any asynchronous construct there is simply no guarantee. If the developer needs to use the result multiple times, they can save it to a scoped variable as the example in this PR shows.

Of course, the embedder is still free to keep objects alive longer than a synchronous execution if it wants.

@sigurdschneider
Copy link

sigurdschneider commented Sep 5, 2019

I would even recommend showing the pattern

const x = wr.deref();
if (x) {
   ...
}

as the canonical example and discuss that the scope of x needs to be controlled, to not keep the object alive too long accidentally.

After that, one can discuss that there are guarantees about the return value of deref in synchronous code.

I think it is more robust to rely on the correct scoping of x than to ensure no intervening await is present: a future change may accidentally introduce such an await, without looking or touching the declaration of x.

@littledan
Copy link
Member

@sigurdschneider @mathiasbynens Are you planning on updating this code sample based on the comments in the review?

@littledan
Copy link
Member

@mathiasbynens Ping, would you be interested in fixing the error I mentioned in #150 (comment) ?

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.

5 participants