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

[WIP] update Holochain-related client deps to latest versions #6

Closed
wants to merge 26 commits into from

Conversation

pospi
Copy link
Collaborator

@pospi pospi commented Jan 21, 2023

Fixes duplicate @holochain/client modules in build causing bundler issues with some other libraries, which is worthwhile doing regardless but will hopefully solve #5.

I've taken the liberty of relaxing dependency specifiers to minver rather than exact versions. This should help to alleviate dependency issues now that upstream is in beta.

Needs more work that I'm not comfortable about proceeding with currently:

  • @holochain-open-dev/cell-client appears to be deprecated and I'm unsure what to replace it with
  • tests/src/provider.ts potentially needs to be updated for any changes to Tryorama

…ate @holochain/client modules in build causing bundler issues
@pospi pospi requested a review from weswalla January 21, 2023 04:32
@weswalla
Copy link
Collaborator

Hey @pospi let me try and help with those two outstanding points:

  1. yes cell-client is deprecated and you can now use @holochain/client to make calls to the zomes.
    I made some changes to the TodoStore & TodoService to take in a websocket and the cell Id, here is the key change from cell-client to @holochain/client: https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-2e30f578d7e2351a885881072271aea34d2a4378d66c077376bec2e5dfb802d9R35-R43
    and seen in the TodoStore constructor: https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-cc76ea8a8cf7f6123e0f68e819c6ada5da04f90526a4b8314c1c2b7e9d581fcdR19-R28
    and setting up the todo store in the applet (not the test-harness): https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-305f00edc8ee54020a7ff963e3e34a86fdc2451680e0c64e49792727e05120e1R30-R47
    Note that I was getting some weird errors when trying to directly use the appWebsocket that nh-we provides to the applet, so I reinstantiate it like so: https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-305f00edc8ee54020a7ff963e3e34a86fdc2451680e0c64e49792727e05120e1R42

in the "test-harness" component: https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-bbb0f3feef854ed8a0e0644c1054becf105cf3162557443eea0ace93b8a70e07R85-R89

  1. yep some changes to tryorama were needed, in bumping the todo applet I got it working, see the following diffs:
    https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-c7273ca77f452520adba2dfb738cd146b83110e7faa7b861318e89b2b56b5d56R24
    https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-98b3adeacf0951ab9b5ee520c638fe14bc2f37d04bb9bf9f7b1b718124129754R12-R16
    (no longer could specify the dna source directly when adding players with happ, had to switch to using a AppBundleSource), https://github.com/neighbour-hoods/todo-applet/pull/2/files#diff-d25304db0d4774a83800a6a6e33dcb3068a119eac9d7dba6d3ec1aa51c1bdd29R7
    And the changes in sensemaker-lite were more significant since we wanted to provide happ properties at runtime to include the CAs pubkey, and the client API had some confusing changes, but this ultimately worked to install the happ with all the properties: https://github.com/neighbour-hoods/sensemaker-lite/pull/22/files#diff-d25304db0d4774a83800a6a6e33dcb3068a119eac9d7dba6d3ec1aa51c1bdd29R46-R92

Hope that helps!

@@ -20,12 +20,11 @@
"uuidv4": "^6.2.11"
},
"devDependencies": {
"@holochain/client": "0.8.0",
"@holochain/tryorama": "0.7.0",
"@holochain/client": "^0.11.14",
Copy link
Collaborator

Choose a reason for hiding this comment

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

so it looks like 0.11.14 is meant for holochain beta-rc.3 (as per default.nix on the tagged commit: https://github.com/holochain/holochain-client-js/blob/35a94fceda77335f14a694ef3a7e6a40e6a20980/default.nix#L4)
but it looks like 0.11.13 is for beta-rc.2: https://github.com/holochain/holochain-client-js/blob/cdcf8496811817f06d96cea471db90890602bd27/default.nix#L4
I'm actually not certain about how strict we need to be with the client versions and how semver is being used in the client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dang it :p Sure, we should wind those back for now.

I'm actually not certain about how strict we need to be with the client versions and how semver is being used in the client.

I would recommend specifying ^0.11.13 and committing the workspace package lockfile. That should allow 0.11.14 to be used once we're ready to update whilst also preventing breakages due to semi-randomly selected "most recent versions" (package-lock.json ensures that any devs will end up with identical packages rather than potentially varied ones).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(still to do: @pospi to organize lockfile)

@weswalla if you can't get it to not download 0.11.14 let me know and I can try to fix. I've never tried it before but I suspect that an npm i on a clean repo will prefer the most recent compatible package from npm (.14) and I thought maybe hacking at package-lock.json after generation to override that to .13 followed by wiping node_modules and another npm i could get us what we want for this version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the issue is that there are breaking changes even when only the rightmost number changes, so if we don't pin an exact client version there may be imcompatibilities with imported modules that also depend on the client (like @neighbourhoods/nh-we-applet)

Copy link
Collaborator

@weswalla weswalla Jan 25, 2023

Choose a reason for hiding this comment

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

so for example, when this is on 0.11.13 then initializing SensemakerStore() gives an error because @neighbourhoods/nh-we-applet is on 0.11.9, which I'm pretty sure is not following semver...

Like AppAgentWebsocket.connect(), in 0.11.9 it is defined like so: https://github.com/holochain/holochain-client-js/blob/66d2e62f1d279f7e12ff6f41e67ad8eac606a55f/src/api/app-agent/websocket.ts#L77-L80
but in 0.11.13, like this: https://github.com/holochain/holochain-client-js/blob/cdcf8496811817f06d96cea471db90890602bd27/src/api/app-agent/websocket.ts#L80-L84

ui/package.json Outdated Show resolved Hide resolved
ui/src/provider-app-test-harness.ts Show resolved Hide resolved
ui/src/provider-app-test-harness.ts Outdated Show resolved Hide resolved
we-applet/src/index.ts Outdated Show resolved Hide resolved
we-applet/src/index.ts Outdated Show resolved Hide resolved
…ecking) Rollup plugin during watched build, fix wrong script path in UI index file
@pospi pospi mentioned this pull request Jan 27, 2023
@weswalla weswalla closed this Jan 27, 2023
@pospi pospi deleted the feature/holochain-dependency-updates branch January 28, 2023 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants