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

define Key, PureData using parameterized Passable (WIP) #1952

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

dckc
Copy link
Contributor

@dckc dckc commented Jan 10, 2024

suggestions for #1933

I think the pass-style stuff is complete. The patterns stuff is just a start.

The marshal stuff is probably superfluous.

@turadg
Copy link
Member

turadg commented Jan 10, 2024

@dckc I rebased #1933 on top of master so I rebased this PR on top of that. https://github.com/endojs/endo/tree/ta/dc-Passable-types has the result.

I don't totally get what it buys. Does it make sense to try getting #1933 into master and coming back to this?

@turadg turadg force-pushed the 1488-Passable branch 3 times, most recently from 1b30d60 to ddcdad1 Compare January 11, 2024 01:01
Base automatically changed from 1488-Passable to master January 11, 2024 01:09
Copy link
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.

This was in my review queue so I took a look at getting this into master and most of it is already, including constraining PureData.

The only functional change I see is:

- * @typedef {Exclude<Passable<RemotableObject, never>, Error | Promise>} Key
+ * @typedef {Passable<RemotableObject, never>} Key

@dckc is that more accurate? What's a good test that fails now and will pass?

@dckc
Copy link
Contributor Author

dckc commented Jul 10, 2024

I think that's not even a functional change. The Exclude<...> is redundant: the never already says that.

So it's just a refactor for clarity/conciseness.

A few type tests to show that the types are equivalent might be worthwhile.

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.

2 participants