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.

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

build: add dependencies for subpackages #87

Merged
merged 6 commits into from
Feb 29, 2024
Merged

build: add dependencies for subpackages #87

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f4a9c42
build: add dependencies for subpackages
samsiegart Feb 27, 2024
723824d
fix(build): fix web-components typescript imports
samsiegart Feb 27, 2024
ab6a346
build: run prepack first in workflows
samsiegart Feb 28, 2024
4a969de
chore(web-components): make rpc devDependency only
samsiegart Feb 28, 2024
77b2002
build: dont run prepack in deploy workflow
samsiegart Feb 28, 2024
367c2e9
build: remove unneeded peerDependencies
samsiegart Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ jobs:
cache: 'yarn'
cache-dependency-path: yarn.lock
- run: yarn install --frozen-lockfile
# Prepack so the linter and tests have access to the build exports of
# other packages' prepack.
- run: yarn workspaces run prepack
turadg marked this conversation as resolved.
Show resolved Hide resolved
- run: yarn lint
- run: yarn test
- run: yarn workspaces run prepack
1 change: 0 additions & 1 deletion .github/workflows/trunk.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ jobs:
cache-dependency-path: yarn.lock

- run: yarn install --frozen-lockfile

Copy link
Member

Choose a reason for hiding this comment

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

ignorable nit: removing this newline makes the comment a little harder to read

# Adapted from https://johnny.sh/notes/publish-canary-lerna-cicd/
- name: configure NPM token
run: |
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@
"@endo/eslint-plugin": "^0.4.4",
"@endo/ses-ava": "^0.2.40",
"@jessie.js/eslint-plugin": "^0.4.0",
"@typescript-eslint/eslint-plugin": "^5.59.11",
"@typescript-eslint/parser": "^5.0.0",
"@typescript-eslint/eslint-plugin": "^7.1.0",
"@typescript-eslint/parser": "^7.1.0",
"ava": "^5.3.0",
"conventional-changelog-conventionalcommits": "^6.0.0",
"eslint": "^8.1.0",
"eslint": "^8.57.0",
"eslint-config-jessie": "^0.0.6",
"eslint-config-prettier": "^8.8.0",
"eslint-plugin-html": "^7.1.0",
Expand Down
4 changes: 4 additions & 0 deletions packages/react-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
"prepack": "yarn build"
},
"dependencies": {
"@agoric/rpc": "^0.9.0",
"@agoric/web-components": "^0.15.0",
"@cosmos-kit/core": "2.8.9",
"@cosmos-kit/react": "2.10.10",
"@interchain-ui/react": "1.21.18",
Expand Down Expand Up @@ -71,6 +73,8 @@
"vitest": "0.34.1"
},
"peerDependencies": {
"@agoric/rpc": "^0.9.0",
"@agoric/web-components": "^0.15.0",
Copy link
Member

Choose a reason for hiding this comment

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

They're either dependencies or peerDependencies but not both.

peer has less burden on the consumer, so if that works go with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they should be dependencies because they're imported and used directly in this package: https://github.com/Agoric/ui-kit/blob/main/packages/react-components/src/lib/context/AgoricProviderLite.tsx#L10-L18

It's possible the consumer doesn't directly use the exposed chainStorageWatcher or walletConnection types from the context so I think it would be more burden to require them to add these as dependencies on their own.

However, if the client is using those types elsewhere, I figured it would make sense to put them in peerDependencies as well to avoid conflicts. Am I thinking about this wrong?

Copy link
Member

Choose a reason for hiding this comment

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

yeah then they're dependencies.

I see your motivation to constrain the peerDependencies as well, but I don't think it's warranted.

"@cosmos-kit/core": "2.8.9",
"@cosmos-kit/react": "2.10.10",
"@interchain-ui/react": "1.21.18",
Expand Down
6 changes: 4 additions & 2 deletions packages/web-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@
"lit": "2.0.2"
},
"devDependencies": {
"@agoric/rpc": "^0.9.0",
Copy link
Member

Choose a reason for hiding this comment

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

I see this is a new dependency as of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I figured it was a dependency at first but then realized it was just using any for the chainStorageWatcher type, but left this in since that should be fixed anyway.

"@babel/plugin-proposal-private-property-in-object": "^7.21.11",
"@custom-elements-manifest/analyzer": "^0.4.17",
"@keplr-wallet/types": "^0.11.37",
"@open-wc/eslint-config": "^10.0.0",
"@rollup/plugin-commonjs": "^25.0.1",
"@web/dev-server": "^0.2.2",
"@web/test-runner": "^0.16.1",
"eslint": "^8.36.0",
"eslint": "^8.57.0",
"eslint-plugin-lit": "^1.8.2",
"eslint-plugin-lit-a11y": "^2.4.0",
"ses": "0.18.7",
Expand All @@ -48,7 +49,8 @@
"extends": [
"@open-wc",
"@agoric",
"plugin:jsdoc/recommended-typescript-flavor-error"
"plugin:jsdoc/recommended-typescript-flavor-error",
"plugin:import/typescript"
],
"rules": {
"jsdoc/require-returns-description": "off",
Expand Down
3 changes: 2 additions & 1 deletion packages/web-components/src/wallet-connection/walletConnection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import { makeAgoricSigner } from './makeAgoricSigner.js';
import { watchWallet } from './watchWallet.js';
import { connectKeplr } from './connectKeplr.js';

/** @typedef {import("@agoric/rpc").ChainStorageWatcher} ChainStorageWatcher */
/** @typedef {import("@cosmjs/stargate").SigningStargateClient} SigningStargateClient */
/** @typedef {{client: SigningStargateClient, address: string }} ClientConfig */

/**
* @param {any} chainStorageWatcher
* @param {ChainStorageWatcher} chainStorageWatcher
* @param {string} rpc
* @param {((error: unknown) => void)} [onError]
* @param {ClientConfig} [clientConfig]
Expand Down
19 changes: 12 additions & 7 deletions packages/web-components/src/wallet-connection/watchWallet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import { makeNotifierKit } from '@agoric/notifier';
import { AmountMath } from '@agoric/ertp';
import { iterateEach, makeFollower, makeLeader } from '@agoric/casting';
import { Tendermint34Client } from '@cosmjs/tendermint-rpc';
import { AgoricChainStoragePathKind } from '@agoric/rpc';
import { Far } from '@endo/marshal';
import { queryBankBalances } from './queryBankBalances.js';

/** @typedef {import("@agoric/rpc").ChainStorageWatcher} ChainStorageWatcher */
/** @typedef {import('@agoric/smart-wallet/src/types.js').Petname} Petname */

/** @typedef {import('@keplr-wallet/types').Coin} Coin */

/**
Expand Down Expand Up @@ -35,7 +37,7 @@ const RETRY_INTERVAL_MS = 200;
const MAX_ATTEMPTS_TO_WATCH_BANK = 2;

/**
* @param {any} chainStorageWatcher
* @param {ChainStorageWatcher} chainStorageWatcher
* @param {string} address
* @param {string} rpc
* @param {((error: unknown) => void)} [onError]
Expand Down Expand Up @@ -83,7 +85,7 @@ export const watchWallet = (
let lastPaths;
let isWalletMissing = false;
chainStorageWatcher.watchLatest(
['data', `published.wallet.${address}.current`],
[AgoricChainStoragePathKind.Data, `published.wallet.${address}.current`],
value => {
if (!value) {
if (!isWalletMissing) {
Expand Down Expand Up @@ -161,7 +163,7 @@ export const watchWallet = (

const watchVbankAssets = () => {
chainStorageWatcher.watchLatest(
['data', 'published.agoricNames.vbankAsset'],
[AgoricChainStoragePathKind.Data, 'published.agoricNames.vbankAsset'],
value => {
vbankAssets = value;
possiblyUpdateBankPurses();
Expand Down Expand Up @@ -204,7 +206,7 @@ export const watchWallet = (

const watchBrands = () => {
chainStorageWatcher.watchLatest(
['data', 'published.agoricNames.brand'],
[AgoricChainStoragePathKind.Data, 'published.agoricNames.brand'],
value => {
agoricBrands = value;
possiblyUpdateNonBankPurses();
Expand All @@ -214,7 +216,10 @@ export const watchWallet = (

const watchPurses = () =>
chainStorageWatcher.watchLatest(
['data', `published.wallet.${address}.current`],
[
AgoricChainStoragePathKind.Data,
`published.wallet.${address}.current`,
],
async value => {
const { purses } = value;
if (nonBankPurses === purses) return;
Expand Down Expand Up @@ -250,7 +255,7 @@ export const watchWallet = (
const leader = makeLeader(rpc);
const follower = makeFollower(`:published.wallet.${address}`, leader, {
proof: 'none',
unserializer: chainStorageWatcher.marshaller,
unserializer: Far('marshaller', chainStorageWatcher.marshaller),
});

for await (const update of iterateEach(follower)) {
Expand Down
7 changes: 4 additions & 3 deletions packages/web-components/test/walletConnection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe('makeAgoricWalletConnection', () => {

// Don't bother faking keplr, just expect it to try to enable and fail.
await expect(() =>
// @ts-expect-error fake partial watcher implementation
Copy link
Member

Choose a reason for hiding this comment

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

you could cast the watcher so you don't have to ignore at each site. up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Casting requires stubbing out more of the functions than the test uses, or else I'm getting type errors. Unless you mean casting to any, but I think the expect-error with explanation would be better.

Copy link
Member

Choose a reason for hiding this comment

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

I meant to put the one ts-expect-error at the cast site. then it will let you lie about what the type is.

i agree that any expect-error should have an explanation

makeAgoricWalletConnection(watcher, rpc),
).rejects.toThrowError(Errors.enableKeplr);
});
Expand All @@ -65,10 +66,10 @@ describe('makeAgoricWalletConnection', () => {
};

const connection = await makeAgoricWalletConnection(
// @ts-expect-error fake partial watcher implementation
watcher,
rpc,
undefined,
// @ts-expect-error fake SigningStargateClient
{ address: testAddress, client: {} },
);

Expand All @@ -88,10 +89,10 @@ describe('makeAgoricWalletConnection', () => {
};

const connection = await makeAgoricWalletConnection(
// @ts-expect-error fake partial watcher implementation
watcher,
rpc,
undefined,
// @ts-expect-error fake SigningStargateClient
{ address: testAddress, client: {} },
);

Expand Down Expand Up @@ -123,10 +124,10 @@ it('submits a spend action', async () => {
};

const connection = await makeAgoricWalletConnection(
// @ts-expect-error fake partial watcher implementation
watcher,
rpc,
undefined,
// @ts-expect-error fake SigningStargateClient
{ address: testAddress, client: {} },
);

Expand Down
Loading
Loading