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

bump to beta-rc.2 #7

Merged
merged 28 commits into from
Jan 31, 2023
Merged

bump to beta-rc.2 #7

merged 28 commits into from
Jan 31, 2023

Conversation

weswalla
Copy link
Collaborator

@pospi
I tried cleaning up the code as best I could, and committed the lock files I had, as this works when I run npm run start.

pospi and others added 24 commits January 21, 2023 14:27
…ate @holochain/client modules in build causing bundler issues
…ecify holochain client 0.11.13"

This reverts commit 8bf64b7.
@weswalla
Copy link
Collaborator Author

@pospi I can successfully build the provider.webhapp file with npm run package and install it inside nh-we. Can you try it out on your computer when you have time?

@pospi
Copy link
Collaborator

pospi commented Jan 27, 2023

I can now get as far as an empty Tauri window with an "app not installed" error. I think that's probably just something minor in package scripts to fix up? But unsure.
data: "Conductor returned an error while using a ConductorApi: AppNotInstalled(\"provider-sensemaker\")"

@pospi
Copy link
Collaborator

pospi commented Jan 27, 2023

Also noting that we should merge #4 along with this one, but discard #6.

@pospi
Copy link
Collaborator

pospi commented Jan 27, 2023

Maybe log as a separate issue, but something we're going to want with this new development setup is sourcemaps. It's pretty much impossible to debug with the minified source...

@pospi
Copy link
Collaborator

pospi commented Jan 27, 2023

It looks like I am in a mostly good place with these changes, sorry they got so conflicted. UI mostly works and the We Applet builds (but I haven't setup an environment to test the built version yet). I think we are close but these still seem like blockers:

If we are going to modify our development pipeline to remove wds from the picture, then we should make that decision carefully and clean up after ourselves.

  • Are we sure? Granted, the build pipeline differences have been problematic. WDS does however make claims about its superior speed, we might be slowing things down dramatically by switching to Rollup. But maybe that is fine in the interests of "get it working" and we can revisit WDS later if we start getting reports about slow build speed?
  • Something else to check: removing WDS means removing the ability to debug in any context other than a Tauri window, which means foregoing Firefox Devtools features and other development tooling plugins. The only way to debug elsewhere is via WDS or similar.

If we're happy with this decision:

  • Remove all WDS usage and configuration from the repo & package scripts to avoid confusion in future
  • Get sourcemaps working with Rollup so that debugging is possible. (I'll have a look at this now.)

@pospi
Copy link
Collaborator

pospi commented Jan 27, 2023

Thinking about it more and bypassing WDS may also mean breaking HMR which is really going to slow down development a lot; would you mind testing with your working app to see if you can successfully hot-swap a component? If that's not working then I think we should put some time into getting WDS working again and run hc launch pointed at a URL rather than a file so that a HMR-capable dev env can run through it.

@pospi
Copy link
Collaborator

pospi commented Jan 27, 2023

Did some more testing and discovered something interesting:

  • provider-app-test-harness connects with the code this.appInfo = await this.appWebsocket.appInfo({ installed_app_id: 'provider-sensemaker' });.
  • I would have then presumed that this.appInfo.installed_app_id would match, but instead it is set to test-app. I can't find this string mentioned anywhere in the codebase so it may be getting inserted by the sandbox somehow.

If I edit the hardcoded app_id in createCloneCell to replace with this.appInfo.installed_app_id it seems to work. But I figured we should double-check why this behaviour is occurring and what the expected behaviour is before committing.

I now have a rendering page in the test applet- I can see by inspecting the DOM that it should be showing all the provider components through to "this is a provider component!" even though for some reason the page still renders as an empty white screen.

@pospi
Copy link
Collaborator

pospi commented Jan 28, 2023

Tested updates to the https://github.com/neighbour-hoods/todo-applet/ today and am hitting the same bug as above with regard to mismatching app IDs.

Though the fix seems simple enough, it would be good to understand where this "test-app" app ID discrepancy comes from. I know that @weswalla is running a non-Nix Cargo-based environment to workaround some upstream bugs in NixOS affecting the Apple M1 chipset; whereas I have a standard Nix env. I am pretty sure we are both running the hc launch command specified in the repository?

@zippy @matthme @jost-s if you have any ideas about what app devs should expect during testing and whether this is a feature or a bug, please let us know?

@weswalla
Copy link
Collaborator Author

yeah I've just been using the hc launch command here:

"launch:happ": "RUST_LOG=warn echo \"pass\" | hc launch --piped -n $AGENTS ./workdir/sensemaker-enabled/provider-sensemaker.happ -w --ui-path ui/dist network mdns",

and in terms of differences between our machines, I did have to also install holochain_cli_launch to be able to run hc launch (holochain/launcher#116 (comment))

@matthme
Copy link

matthme commented Jan 28, 2023

In an older version of hc launch all apps where installed with the app id "test-app". However, since a while now they are installed with the app id extracted from the .happ file name: https://github.com/holochain/launcher/blob/8a3be212c443e9a64b8862b30f4f27d192b56053/crates/hc_launch/src-tauri/src/cli.rs#L157

So if you have one of the latest versions of hc launch the only explanation I have for it is that the .happ you're pointing to is called test-app.happ.

The latest version of hc launch is on crates.io now by the way (for 0.1.0-beta): https://crates.io/crates/holochain_cli_launch

@pospi
Copy link
Collaborator

pospi commented Jan 30, 2023

Hmm sorry @weswalla I think there are still some issues with the init code.

On first run, createCloneCell executes successfully and I get a working connection. Then I edit some script and the Tauri window reloads, sending execution through the "already cloned" code path. But it then fails at this._sensemakerStore.registerApplet with "No cell found with role_name sensemaker-clone".

Maybe this is another "older version of hc launch" error, not sure if you're experiencing the same bug or not?

@pospi
Copy link
Collaborator

pospi commented Jan 30, 2023

@weswalla it looks as though if this line is assigned from clonedSensemakerCell.clone_id instead of clonedSensemakerCell.name it inits correctly.

Can I get you to test that before committing & merging this? Just want to be sure it works for both of us rather than being a result of launcher feature divergences.

@pospi
Copy link
Collaborator

pospi commented Jan 30, 2023

(one more, sorry!) I think probably also the initialiser for appAgentWebsocket should be using this.appInfo.installed_app_id instead of "todo-sensemaker".

@weswalla
Copy link
Collaborator Author

@pospi I added your suggested changes and everything works. npm run start works, and npm run package along with testing the provider.webhapp inside nh-we. I'd say ready to merge.

@weswalla
Copy link
Collaborator Author

I am unable to approve my own PR. @julioholon can you approve? or @pospi do you have permission to approve?

@pospi pospi self-requested a review January 31, 2023 01:19
@pospi
Copy link
Collaborator

pospi commented Jan 31, 2023

I added myself as a reviewer and approved it, hopefully it'll let you merge now?

@weswalla weswalla merged commit 499088d into main Jan 31, 2023
@weswalla weswalla deleted the fixing-holochain-client branch January 31, 2023 01:24
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.

3 participants