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

fix(patterns): type guards #1715

Merged
merged 1 commit into from
Aug 21, 2023
Merged

fix(patterns): type guards #1715

merged 1 commit into from
Aug 21, 2023

Conversation

erights
Copy link
Contributor

@erights erights commented Aug 9, 2023

Subset of #1712 that does not break compat.

Typing and validation for InterfaceGuards, MethodGuards, and AwaitArgGuards.

@erights
Copy link
Contributor Author

erights commented Aug 15, 2023

@michaelfig , making you a reviewer because I'm staging my interface-opt-out experiment on this. Let me know if you see a problem with that. I so, I can remove the dependency, but I'd rather not.

@erights
Copy link
Contributor Author

erights commented Aug 15, 2023

Note that I'd prefer to stage on #1712 if that were feasible. But I'm scared of the compat break. Eager for your feedback on that too.

@erights erights requested a review from turadg August 15, 2023 23:02
@erights
Copy link
Contributor Author

erights commented Aug 15, 2023

@turadg Making you a reviewer as I know you've done previous work typing guards. I don't know how that relates to the contents of this PR. What should I look at?

@michaelfig
Copy link
Member

Note that I'd prefer to stage on #1712 if that were feasible. But I'm scared of the compat break. Eager for your feedback on that too.

I believe the breaking change in #1712 won't impact any deployed code. IIUC, that's the change of guards from copyRecords to tagged payloads. I can't think of any instance where we put guards in object state or Stores in general, or send them over the network.

If that seems correct to you, I'd encourage you to make the change and stage on #1712. I had just one review comment on #1712, and would be happy to approve after you consider that question.

@erights
Copy link
Contributor Author

erights commented Aug 16, 2023

Since #1712 is staged on this, even if we decide we can merge #1712, it doesn't hurt to first merge this PR, once it meets your approval.

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Approval based on your addressing my comments.

packages/patterns/src/patterns/patternMatchers.js Outdated Show resolved Hide resolved
packages/patterns/src/patterns/patternMatchers.js Outdated Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Aug 17, 2023

Note that I'd prefer to stage on #1712 if that were feasible. But I'm scared of the compat break. Eager for your feedback on that too.

I believe the breaking change in #1712 won't impact any deployed code. IIUC, that's the change of guards from copyRecords to tagged payloads. I can't think of any instance where we put guards in object state or Stores in general, or send them over the network.

If that seems correct to you, I'd encourage you to make the change and stage on #1712. I had just one review comment on #1712, and would be happy to approve after you consider that question.

AFAICT, you're correct re durable state and network messages. However, #1712 seems incompat with some of the code in current agoric-sdk. For example:

image

@erights erights merged commit 39d9031 into master Aug 21, 2023
13 checks passed
@erights erights deleted the markm-type-guards branch August 21, 2023 22:58
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