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.

Sign up for GitHub

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

Jump to bottom

Minimal proposal spec #12

Merged
merged 2 commits into from
Jul 14, 2022
Merged

Minimal proposal spec #12

merged 2 commits into from
Jul 14, 2022

Conversation

Jack-Works
Copy link
Member

@Jack-Works Jack-Works commented Apr 7, 2022 •
edited
Loading

close #8

image

image

image

image

@acutmore
Copy link

acutmore commented Apr 7, 2022

Only having the isRevoked getter and not a way to listen for the revoke call seems like it would add a delay to when the target and handler references are dropped allowing them to be collected?

@ajvincent
Copy link
Collaborator

I think having Proxy.revocable() not always return {proxy, revoke} is something we can not ask for. Though it may seem weird, I can foresee someone out there wanting to still revoke an individual proxy, while letting hundreds of others live. Plus, it's a change in what the function actually returns from a generic dictionary to a Proxy.

So the algorithm at 1.3 step 3b has to go, and the "Else" branch at 1.4 has to be unconditionally run, I think.

ajvincent
ajvincent requested changes Apr 7, 2022
View reviewed changes
Copy link
Collaborator

@ajvincent ajvincent left a comment

Choose a reason for hiding this comment

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

I don't think we can change the return signature of Proxy.revocable.

@ajvincent
Copy link
Collaborator

Only having the isRevoked getter and not a way to listen for the revoke call seems like it would add a delay to when the target and handler references are dropped allowing them to be collected?

Observation was dropped / deferred to another time, per #4. As for why, the brief explanation at the bottom of our README gives the rationale.

@Jack-Works
Copy link
Member Author

The current semantics allows the engine to GC all targets & handlers of the proxy.

I think having Proxy.revocable() not always return {proxy, revoke} is something we can not ask for.

What about changing Proxy.revocable to return { proxy: T, revoke?: Function }? I prefer adding this functionality to the Proxy.revocable instead of new Proxy because it has better semantics.

@Jack-Works
Copy link
Member Author

Though it may seem weird, I can foresee someone out there wanting to still revoke an individual proxy, while letting hundreds of others live.

Oh... This is definitely reasonable. So the correct choice is: accept it on both Proxy.revocable and new Proxy (as 3rd argument)

@ajvincent
Copy link
Collaborator

The current semantics allows the engine to GC all targets & handlers of the proxy.

I think having Proxy.revocable() not always return {proxy, revoke} is something we can not ask for.

What about changing Proxy.revocable to return { proxy: T, revoke?: Function }? I prefer adding this functionality to the Proxy.revocable instead of new Proxy because it has better semantics.

This proposal is definitely aiming at Proxy.revocable, and considering new Proxy in #6.

@Jack-Works
Copy link
Member Author

spec updated.

@ajvincent ajvincent self-requested a review April 9, 2022 17:27
ajvincent
ajvincent approved these changes Apr 9, 2022
View reviewed changes
Copy link
Collaborator

@ajvincent ajvincent left a comment

Choose a reason for hiding this comment

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

You've answered my objection. I'll wait for the others to review before merging, for now.

@ajvincent
Copy link
Collaborator

I still have a concern about directly referencing AbortController as the mass-revocation signal basis.

@Jack-Works
Copy link
Member Author

Maybe we can follow tc39/proposal-cancellation#31 as our foundation.

@ajvincent
Copy link
Collaborator

There's a conflict. Can you please update this PR to account for that?

@Jack-Works
Copy link
Member Author

@ajvincent done

ajvincent
ajvincent reviewed Jul 14, 2022
View reviewed changes
index.html
<emu-clause id="sec-proxy.revocable" oldids="sec-proxy-revocation-functions"><span id="sec-proxy-revocation-functions"></span>
<h1><span class="secnum">1.3</span> Proxy.revocable ( <var>target</var>, <var>handler</var>, <var>options</var> )</h1>
<p>The <code>Proxy.revocable</code> function is used to create a revocable Proxy object. When <code>Proxy.revocable</code> is called with arguments <var>target</var>, <var>handler</var> and <var>options</var>, the following steps are taken:</p>
<emu-alg><ol><li>Let <var>p</var> be ?&nbsp;<emu-xref aoid="ProxyCreate"><a href="https://tc39.es/ecma262/#sec-proxycreate">ProxyCreate</a></emu-xref>(<var>target</var>, <var>handler</var>).</li><li><ins>Let <var>signal</var> be ?&nbsp;<emu-xref aoid="GetProxySignal" id="_ref_0"><a href="#sec-get-proxy-signal">GetProxySignal</a></emu-xref>(<var>options</var>).</ins></li><li><ins>If <var>signal</var> is not <emu-val>undefined</emu-val>, then</ins><ol><li><ins>Let <var>p</var>.[[RevocationSignal]] be <var>signal</var>.[[HostRevocationSignal]].</ins></li></ol></li><li>Let <var>revokerClosure</var> be a new <emu-xref href="#sec-abstract-closure"><a href="https://tc39.es/ecma262/#sec-abstract-closure">Abstract Closure</a></emu-xref> with no parameters that captures nothing and performs the following steps when called:<ol><li>Let <var>F</var> be the <emu-xref href="#active-function-object"><a href="https://tc39.es/ecma262/#active-function-object">active function object</a></emu-xref>.</li><li>Let <var>p</var> be <var>F</var>.[[RevocableProxy]].</li><li>If <var>p</var> is <emu-val>null</emu-val>, return <emu-val>undefined</emu-val>.</li><li>Set <var>F</var>.[[RevocableProxy]] to <emu-val>null</emu-val>.</li><li><emu-xref href="#assert"><a href="https://tc39.es/ecma262/#assert">Assert</a></emu-xref>: <var>p</var> is a Proxy object.</li><li>Set <var>p</var>.[[ProxyTarget]] to <emu-val>null</emu-val>.</li><li>Set <var>p</var>.[[ProxyHandler]] to <emu-val>null</emu-val>.</li><li><ins>Set <var>p</var>.[[RevocationSignal]] to <emu-val>null</emu-val>.</ins></li><li>Return <emu-val>undefined</emu-val>.</li></ol></li><li>Let <var>revoker</var> be <emu-xref aoid="CreateBuiltinFunction"><a href="https://tc39.es/ecma262/#sec-createbuiltinfunction">CreateBuiltinFunction</a></emu-xref>(<var>revokerClosure</var>, 0, <emu-val>""</emu-val>, « [[RevocableProxy]] »).</li><li>Set <var>revoker</var>.[[RevocableProxy]] to <var>p</var>.</li><li>Let <var>result</var> be <emu-xref aoid="OrdinaryObjectCreate"><a href="https://tc39.es/ecma262/#sec-ordinaryobjectcreate">OrdinaryObjectCreate</a></emu-xref>(<emu-xref href="#sec-properties-of-the-object-prototype-object"><a href="https://tc39.es/ecma262/#sec-properties-of-the-object-prototype-object">%Object.prototype%</a></emu-xref>).</li><li>Perform !&nbsp;<emu-xref aoid="CreateDataPropertyOrThrow"><a href="https://tc39.es/ecma262/#sec-createdatapropertyorthrow">CreateDataPropertyOrThrow</a></emu-xref>(<var>result</var>, <emu-val>"proxy"</emu-val>, <var>p</var>).</li><li>Perform !&nbsp;<emu-xref aoid="CreateDataPropertyOrThrow"><a href="https://tc39.es/ecma262/#sec-createdatapropertyorthrow">CreateDataPropertyOrThrow</a></emu-xref>(<var>result</var>, <emu-val>"revoke"</emu-val>, <var>revoker</var>).</li><li>Return <var>result</var>.</li></ol></emu-alg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if you updated the spec text so step 4h, which sets the [[RevocationSignal]] to null despite step 3a.

I'll bet it's standard for the spec editing code to put all of this on one line, but it's really hard on the eyes to have to scroll so far in to fix a bug on one line.

I will merge with this bug in, and I ask you to write a follow-up PR to fix the bug.

Per #6, I'd also like a follow-up to modify new Proxy() as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see it's generated from spec.emu below. I didn't know that.

@ajvincent ajvincent merged commit 2dfb1ab into main Jul 14, 2022
@ajvincent ajvincent deleted the smallest-proposal branch July 14, 2022 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajvincent ajvincent ajvincent approved these changes

@bmeck bmeck Awaiting requested review from bmeck

@erights erights Awaiting requested review from erights

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A more minimal version
3 participants
@Jack-Works @acutmore @ajvincent