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

refactor(swingset-vat): move VatData to new vat-data package #4603

Merged
merged 5 commits into from
Mar 23, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Feb 18, 2022

closes: #4255 #4830

Description

For #4830 creates a new @agoric/vat-data package to hold the module that wraps the VatData global so that a) it need not be deep-imported and b) it can be imported without depending on the whole @agoric/swingset-vat package in builds. (Sourced only in devDependencies for use in tests.)

For #4255 replaces the use of globals.d.ts with imports from that package.

Shrinks the concessions needed for #4618 (comment)

Defines types for the VatData methods.

Security Considerations

This creates a layer of indirection to access the VatData methods. Whereas the caller previously would request it directly from globalThis now it's requested from a module in a new package which exposes the global value, opening up a supply chain attack.

Documentation Considerations

Increased documentation. Fully backwards compatible.

Testing Considerations

The package has new Ava tests. It also has a type test at src/index.test-d.js per the pattern in https://github.com/SamVerschueren/tsd, though it doesn't make use of the tsd helpers or runner.

@turadg turadg changed the title refactor(swingset-vat): export VatData refactor(swingset-vat): move VatData to new vat-data package Mar 23, 2022
@turadg turadg marked this pull request as ready for review March 23, 2022 22:16
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

I like what I see. I don't have enough of an understanding of this to be the approver

Comment on lines +9 to +16
interface KindDefiner {
<P, S, K>(
tag: string,
init: (...args: P) => S,
actualize: (state: S) => K,
finish?: () => void,
): (...args: P) => K;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

the packages/xsnap stuff looks fine

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

¡Muy bueno!

@turadg turadg added the automerge:squash Automatically squash merge label Mar 23, 2022
@mergify mergify bot merged commit d5f9cad into master Mar 23, 2022
@mergify mergify bot deleted the ta/4255-VatData-module branch March 23, 2022 23:49
@mhofman
Copy link
Member

mhofman commented Mar 24, 2022

For #4255 replaces the use of globals.d.ts with imports from that package.

I'm late to the party here, but I've been trying to follow the following pattern when switching to exported types:

  • all exported types should be available through the base package name. E.g. import('@agoric/vat-data').VatData
  • no longer have any exported.js which bring in ambient types

The solution to the first one is usually to have an export * from './src/types.js' in the index.js entrypoint.

Here vat-data doesn't export its types at all. Since it defines its types as a .d.ts file, it can't be used as an export statement directly, and we'd need an intermediate .js file which does a bunch of /** @typedef {import('./types').Foo} Foo */. In general I'd prefer if we stuck to JSDoc as much as possible for now, since most contributors aren't able to maintain pure TS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

how to expose the Virtual/Durable Collection API? global? module import?
5 participants