Skip to content

feat(pass-style): far class instances#1766

Closed
erights wants to merge 2 commits intomasterfrom
markm-remotable-class-instances
Closed

feat(pass-style): far class instances#1766
erights wants to merge 2 commits intomasterfrom
markm-remotable-class-instances

Conversation

@erights
Copy link
Copy Markdown
Contributor

@erights erights commented Sep 12, 2023

Support use of JS classes to define far objects as class instances.

This was already possible, as demonstrated by prior tests in test-passStyleOf.js . This PR merely provides a base class to make this easier, and tests it.

@erights erights self-assigned this Sep 12, 2023
@erights erights force-pushed the markm-remotable-class-instances branch 2 times, most recently from 6dcc159 to 7e147ae Compare September 12, 2023 05:12
@erights erights requested review from mhofman and turadg September 12, 2023 05:55
Copy link
Copy Markdown
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Stoked to see this working


t.assert(new fb.constructor() instanceof FarBaseClass);
t.throws(() => fb.constructor(), {
// TODO message depends on JS engine, and so is a fragile golden test
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how can this TODO be solved?

isn't it fair to assume all of Endo's tests run in whatever its CI matrix covers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Attn @kriskowal @warner

To truly not rely on any unspecified JS, all I may test is the error type. But I do like ensuring that the test case is failing for the reason it should. I don't know how to do that without this fragile dependency.

Recommendations?

@erights erights force-pushed the markm-remotable-class-instances branch from 7e147ae to 5a2c77d Compare September 13, 2023 03:13
@erights erights requested a review from turadg September 13, 2023 03:55
Copy link
Copy Markdown
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

My main concern with this PR is this, and the difference with ExoClass. Since there is no similar way to handle state, users will simply rely on private fields, which we can't prevent. Also they are free to add accessors, which won't satisfy passStyleOf checks.

I would prefer supporting class syntax in a way compatible with our encapsulation paradigm (instance private state, not class scope private state), and making sure we fail early for class definitions that are not actually remotable.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 18, 2023

See also #1773

@erights erights force-pushed the markm-remotable-class-instances branch from 0ac91dc to 3025328 Compare September 18, 2023 08:21
@erights erights force-pushed the markm-remotable-class-instances branch 3 times, most recently from 490866e to f4d09fc Compare September 27, 2023 23:27
@erights erights force-pushed the markm-remotable-class-instances branch from f4d09fc to dd0088f Compare October 3, 2023 20:28
@erights
Copy link
Copy Markdown
Contributor Author

erights commented Oct 7, 2023

Closing in favor of #1809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants