h-REA / hREA

A ValueFlows / REA economic network coordination system implemented on Holochain and with supplied Javascript GraphQL libraries
https://docs.hrea.io
Other
142 stars 15 forks source link

fix input object type signature for vf-ui/graphql-client-holochain #350

Closed Connoropolous closed 1 year ago

Connoropolous commented 1 year ago

Both 'conductorUri' and 'dnaConfig' should be optional, but aren't currently. Since that makes everything optional, the whole options object should be optional

This is important because this is the primary entry point for app devs

Connoropolous commented 1 year ago

Screen Shot 2022-08-05 at 7 23 14 AM

Connoropolous commented 1 year ago

Closely related:

https://github.com/h-REA/hREA/blob/285148d36293ef318bdd8504aebc093c70ee5eb4/modules/graphql-client/index.ts#L32-L33 This code is buggy because it fails to handle when conductorUri value is empty, not passed, and actually set by the environment variable.

it should take conductorUri back out of the result, and assign it to the options object

Connoropolous commented 1 year ago

@pospi FYI these are some super top priority fixes for me that I plan to work on, that fix the API for graphql-client-holochain

pospi commented 1 year ago

This code is buggy because it fails to handle when conductorUri value is empty, not passed, and actually set by the environment variable.

I'm not sure that's true. If the value is undefined, it shouldn't matter. Agree the empty object is a problem.

So the call signature for the main client entrypoint becomes options?: ClientOptions and we add an initialiser for options || (options = {}).

From there I think it should be fine- bindSchema opens a connection with the same default value logic for conductorUri that autoConnect has. But if you want to be explicit about it you could also let { dnaConfig, conductorUri } = await autoConnect(options.conductorUri) and then assign it back into the original options before passing to initGraphQLClient- that way relies on fewer unrelated code paths.

Connoropolous commented 1 year ago

I did a pass on this today @pospi and that's up in #353 .

"I'm not sure that's true. If the value is undefined, it shouldn't matter. " It was a problem because in graphql-client, there was stack trace where you have connect -> initGraphQLClient -> bindSchema -> generateResolvers -> openConnection and so it bypassed the intelligence of autoConnect at picking up the environment variable version of the value. I experienced this first-hand by trying to integrate graphql-client-holochain into my hrea-dashboard testbed project.

Anyways it's all fixed up in the PR

pospi commented 1 year ago

That is desired functionality though in some certain cases, where the application might want to manually connect to some specific application cells. In those cases you would do so by calling openConnection and then using the same conductorURI when calling zomeFunction later. I think this will still be possible with the changes you've made but just want to note for clarity. This is important if eg. your application has multiple collaboration spaces and you want to have multiple GraphQL APIs active.

Connoropolous commented 1 year ago

This is the issue I was having, that I am fixing. It throws an error so I'm certain it's not desired behaviour, what I'm talking about. Screen Shot 2022-08-07 at 6 03 02 AM

What it sounds like you're talking about is being able to set conductorUri value, and have it be respected, which will work except for in Launcher contexts because AppWebsocket agressively overrides given values with detected values.

But within hREA we are allowing for 'a direct given config value' to override a 'global env var value'