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

Require virtual object selves to be declared Far #3566

Merged
merged 4 commits into from
Jul 31, 2021
Merged

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Jul 30, 2021

Fixes bug #3562

@FUDCo FUDCo added bug Something isn't working SwingSet package: SwingSet labels Jul 30, 2021
@FUDCo FUDCo requested review from warner and erights July 30, 2021 08:27
@FUDCo FUDCo self-assigned this Jul 30, 2021
Copy link
Member

@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.

What about a negative test, where the new assert is violated?

@warner
Copy link
Member

warner commented Jul 30, 2021

BTW I think it's accurate to say "Representatives are Far" rather than trying to define a "virtual object self": We use Representative and Presence and Remotable to refer to the three concrete JS Objects that userspace code interacts with. The "virtual object" is the abstract object with state and behavior, which has a longer lifetime than any single ephemeral+concrete Representative.

At least, that's the terminology that I use (and that I think we need) from everywhere except the interior of the virtual object manager. I guess I'm thinking the initialRepresentative variable is slightly misnamed: the user-provided "kind constructor" returns a record with two pieces (an init function for making new virtual objects, and a self behavior record which becomes the actual concrete Representative Object), but the overall record is being stored in initialRepresentative

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

In addition to @erights 's recommended negative test, please also update docs/virtual-objects.md to include the Far requirement.

Apart from that the code changes look great.

@FUDCo FUDCo force-pushed the 3562-enforce-vo-farhood branch 2 times, most recently from 4a549d4 to af59fcd Compare July 31, 2021 07:07
@FUDCo FUDCo force-pushed the 3562-enforce-vo-farhood branch from af59fcd to f153923 Compare July 31, 2021 09:03
@FUDCo FUDCo requested review from erights and warner July 31, 2021 09:04
Copy link
Member

@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.

LGTM

@FUDCo FUDCo merged commit 63a5236 into master Jul 31, 2021
@FUDCo FUDCo deleted the 3562-enforce-vo-farhood branch July 31, 2021 19:44
@FUDCo FUDCo mentioned this pull request Jul 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants