Skip to content

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

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

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

Conversation

@erights

@erights erights commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

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

@turadg turadg left a comment

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.

Stoked to see this working

Comment thread packages/pass-style/src/far-class-instances.js Outdated
Comment thread packages/pass-style/src/far-class-instances.js Outdated

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

@mhofman mhofman left a comment

Copy link
Copy Markdown
Contributor

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

erights commented Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

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

erights commented Oct 7, 2023

Copy link
Copy Markdown
Contributor Author

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