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

chore: export @agoric/store types #10917

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

sirtimid
Copy link
Contributor

refs: #6343

Description

Properly expose TypeScript definitions from @agoric/store package by adding TypeScript build steps in package.json which generate the definitions.

Security Considerations

No security impact - only affects TypeScript type definitions which are stripped at compile time.

Scaling Considerations

No scaling impact - changes only affect development-time TypeScript support.

Documentation Considerations

No documentation updates needed - this is a developer-facing change for TypeScript users.

Testing Considerations

Existing type coverage tests continue to pass. No additional testing needed as this only affects type definition exposure.

Upgrade Considerations

No upgrade impact - purely development-time TypeScript support changes.

@sirtimid sirtimid requested a review from a team as a code owner January 30, 2025 19:32
@sirtimid sirtimid requested a review from AgoricTriage January 30, 2025 19:32
@mhofman mhofman requested review from turadg and mhofman January 30, 2025 19:46
"../../tsconfig-build-options.json"
],
"exclude": [
// exported.d.ts already exists and it shouldn't be overwritten
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary here and not in @agoric/zoe?

Copy link
Contributor Author

@sirtimid sirtimid Jan 30, 2025

Choose a reason for hiding this comment

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

https://github.com/Agoric/agoric-sdk/blob/master/packages/zoe/tsconfig.json doesn't include exported.js whereas https://github.com/Agoric/agoric-sdk/blob/master/packages/store/tsconfig.json does, so I need to exclude it. Another solution could be to remove it from tsconfig includes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. I suppose it was in includes so it would be checked.

On balance, I think removing it from the includes is the better option because it's more consistent and suffices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey dokey :) 72b2080

@sirtimid sirtimid requested a review from turadg January 30, 2025 21:00
"lint:eslint": "eslint ."
"lint:eslint": "eslint .",
"prepack": "tsc --build tsconfig.build.json",
"postpack": "git clean -f ':!exported.d.ts' '*.d.ts*' '*.tsbuildinfo'"
Copy link
Member

Choose a reason for hiding this comment

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

please make this consistent as well:

Suggested change
"postpack": "git clean -f ':!exported.d.ts' '*.d.ts*' '*.tsbuildinfo'"
"postpack": "git clean -f '*.d.ts*' '*.tsbuildinfo'",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, of course! Ready 5d590aa I've made everything one commit.

@sirtimid sirtimid force-pushed the sirtimid/export-store-types branch from 72b2080 to 5d590aa Compare January 30, 2025 21:07
@sirtimid sirtimid requested a review from turadg January 30, 2025 21:08
@turadg turadg added automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR labels Jan 30, 2025
Copy link
Contributor

mergify bot commented Jan 30, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@turadg turadg force-pushed the sirtimid/export-store-types branch from 5d590aa to 12d7e38 Compare January 30, 2025 23:27
@mergify mergify bot merged commit 16940ff into Agoric:master Jan 30, 2025
76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants