build: add dependencies for subpackages #87

samsiegart · 2024-02-27T12:18:31Z

The publish NPM workflow is failing https://github.com/Agoric/ui-kit/actions/runs/8053306725/job/21995398192

It seems like it's not building and linking the packages in the order according to

Lines 5 to 9 in 2ae3e21

    
           "workspaces": [ 
        
             "packages/ui-components", 
        
             "packages/rpc", 
        
             "packages/web-components", 
        
             "packages/react-components"

, so @agoric/react-components fails to build because @agoric/web-components isn't built yet. It seems like the way to fix this is to declare the dependencies on other sub-packages in their package.jsons, which seems to be what agoric-sdk does for its sub-packages, but we'll see what happens when the workflow runs.

I also replaced an any type with the proper ChainStorageWatcher type in @agoric/web-components, now that the dependency is included.

turadg · 2024-02-28T15:30:31Z

packages/react-components/package.json

+    "@agoric/rpc": "^0.9.0",
+    "@agoric/web-components": "^0.15.0",


They're either dependencies or peerDependencies but not both.

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

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?

yeah then they're dependencies.

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

turadg · 2024-02-28T15:32:17Z

packages/web-components/package.json

@@ -30,14 +30,15 @@
    "lit": "2.0.2"
  },
  "devDependencies": {
+    "@agoric/rpc": "^0.9.0",


I see this is a new dependency as of this PR

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.

turadg · 2024-02-28T15:33:16Z

packages/web-components/test/walletConnection.test.js

@@ -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


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

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.

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

turadg · 2024-02-28T15:34:35Z

.github/workflows/trunk.yml

@@ -21,7 +21,7 @@ jobs:
          cache-dependency-path: yarn.lock

      - run: yarn install --frozen-lockfile
-
+      - run: yarn workspaces run prepack


this shouldn't be necessary. lerna publish will pack along the way. I suppose you had it here for debugging? please either remove or add a comment explaining why this redundant command is included

This was to be safe because the lerna publish wasn't prepacking in the correct order and failing as seen in https://github.com/Agoric/ui-kit/actions/runs/8053306725/job/21995398192

I read that lerna would pack in the correct order if we explicitly added the dependencies in the package.jsons, but added this to be safe because I noticed MAINTAINERS.md includes an explicit prepack step as well. Just took this back out though so we can be sure if it's needed, but worst case we'll have to do another PR to add it back.

.github/workflows/ci.yml

turadg

I leave the outstanding items to your judgement

turadg · 2024-02-28T18:50:13Z

packages/react-components/package.json

+    "@agoric/rpc": "^0.9.0",
+    "@agoric/web-components": "^0.15.0",


yeah then they're dependencies.

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

turadg · 2024-02-28T18:51:35Z

packages/web-components/test/walletConnection.test.js

@@ -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


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

turadg · 2024-02-28T18:52:10Z

.github/workflows/trunk.yml

@@ -21,7 +21,6 @@ jobs:
          cache-dependency-path: yarn.lock

      - run: yarn install --frozen-lockfile
-


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

samsiegart added 4 commits February 27, 2024 04:12

build: add dependencies for subpackages

f4a9c42

fix(build): fix web-components typescript imports

723824d

build: run prepack first in workflows

ab6a346

chore(web-components): make rpc devDependency only

4a969de

samsiegart marked this pull request as ready for review February 28, 2024 05:32

samsiegart requested a review from turadg February 28, 2024 05:32

turadg reviewed Feb 28, 2024

View reviewed changes

build: dont run prepack in deploy workflow

77b2002

samsiegart requested a review from turadg February 28, 2024 18:47

turadg approved these changes Feb 28, 2024

View reviewed changes

build: remove unneeded peerDependencies

367c2e9

samsiegart merged commit b8fd8f1 into main Feb 29, 2024
1 check passed

samsiegart deleted the add-workspace-dependencies branch February 29, 2024 07:26

otoole-brendan mentioned this pull request Mar 6, 2024

Connect wallet button component Agoric/agoric-sdk#8813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add dependencies for subpackages #87

build: add dependencies for subpackages #87

samsiegart commented Feb 27, 2024 •

edited

Loading

turadg Feb 28, 2024

samsiegart Feb 28, 2024

turadg Feb 28, 2024

turadg Feb 28, 2024

samsiegart Feb 28, 2024

turadg Feb 28, 2024

samsiegart Feb 28, 2024

turadg Feb 28, 2024

turadg Feb 28, 2024

samsiegart Feb 28, 2024

turadg left a comment

turadg Feb 28, 2024

turadg Feb 28, 2024

turadg Feb 28, 2024

	"workspaces": [
	"packages/ui-components",
	"packages/rpc",
	"packages/web-components",
	"packages/react-components"

		"@agoric/rpc": "^0.9.0",
		"@agoric/web-components": "^0.15.0",

		@@ -21,7 +21,6 @@ jobs:
		cache-dependency-path: yarn.lock

		- run: yarn install --frozen-lockfile

build: add dependencies for subpackages #87

build: add dependencies for subpackages #87

Conversation

samsiegart commented Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart commented Feb 27, 2024 •

edited

Loading