Skip to content

refactor(pass-style): minor types gardening #2746

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

refactor(pass-style): minor types gardening #2746

wants to merge 2 commits into from

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 22, 2025

Most PRs should close a specific Issue. All PRs should at least reference one or more Issues. Edit and/or delete the following lines as appropriate (note: you don't need both refs and closes for the same one):

Closes: #XXXX
Refs: #XXXX

Description

Add a description of the changes that this PR introduces and the files that are the most critical to review.

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

Scaling Considerations

Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Compatibility Considerations

Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?

Upgrade Considerations

What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?

Include *BREAKING*: in the commit message with migration instructions for any breaking change.

Update NEWS.md for user-facing changes.

Delete guidance from pull request description before merge (including this!)

@erights erights self-assigned this Mar 22, 2025
erights
erights commented Mar 22, 2025
View reviewed changes
packages/pass-style/src/types.d.ts Outdated
@@ -1,4 +1,5 @@
/* eslint-disable no-use-before-define */
import type { Checker as _Checker } from '@endo/common/ident-checker.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this, together with line 208 below export type Checker = _Checker;, really the best way in type script to import-and-reexport a type from elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

I defer to @turadg. I don’t recall whether this was a specific limitation for ambient types.

Copy link
Member

Choose a reason for hiding this comment

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

There's a simpler way. I just pushed it.

@erights erights force-pushed the markm-minor-types-gardening branch from cfb62c3 to 84f90d8 Compare March 22, 2025 19:26
@erights erights force-pushed the markm-minor-types-gardening branch from 84f90d8 to d82aef8 Compare April 8, 2025 00:12
@erights erights force-pushed the markm-minor-types-gardening branch from d82aef8 to 7d4cd1f Compare May 17, 2025 01:37
kriskowal
kriskowal requested changes Jun 3, 2025
View reviewed changes
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Good enough in my book, but it would be lovely to have a types.test-d.ts to validate these type signatures get exported as intended and so we don’t regress. There are some examples of test-d.ts elsewhere in the repo.

@erights erights force-pushed the markm-minor-types-gardening branch from 7d4cd1f to f34669d Compare June 3, 2025 23:34
turadg
turadg approved these changes Jun 9, 2025
View reviewed changes
packages/pass-style/src/types.d.ts Outdated
@@ -1,4 +1,5 @@
/* eslint-disable no-use-before-define */
import type { Checker as _Checker } from '@endo/common/ident-checker.js';
Copy link
Member

Choose a reason for hiding this comment

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

There's a simpler way. I just pushed it.

@erights erights force-pushed the markm-minor-types-gardening branch 4 times, most recently from 5fd6c37 to 912e36c Compare June 10, 2025 17:31
@erights erights mentioned this pull request Jun 10, 2025
Draft
@gibson042
Copy link
Contributor

Possibly relevant: #2851 (comment) (cf. #2285)

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

@kriskowal kriskowal kriskowal requested changes

@turadg turadg turadg approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@erights erights

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erights @gibson042 @turadg @kriskowal