Skip to content
This repository was archived by the owner on Jul 10, 2025. It is now read-only.

Conversation

@coder11
Copy link
Contributor

@coder11 coder11 commented Feb 15, 2023

New API looks like this:

import { Fluence } from '@fluencelabs/js-client.api';

// Somewhere at the root of application
const relay = ...
Fluence.connect(relay);

// If you want to specify client options
Fluence.connect(relay, { keyPair: ..., ... });

// To listen for connection state changes:
// possible states are: "disconnected", "connecting", "connected",	"disconnecting"
Fluence.onConnectionStateChange((state) => console.info('connection state changed: ', state));

// To disconnect from the network:
Fluence.disconnet()

// Both Fluence.connect and Fluenct.disconnect return promises
// which can be used to wait for the operation to fullfill:
const sequential = () => {
   await Fluence.connect(...);
   await runSomeAquaFunction(...)
}

// It is also possible to get the underlying JS Client:
const client = await Fluence.getClient();

console.log('my peer id: ', client.getPeerId());
console.log('my sk id: ', fromByteArray(client.getPeerSecretKey()));

The interface for JS Client is the following:

export interface IFluenceClient {
    /**
     * Connect to the Fluence network
     * @param relay - relay node to connect to
     * @param options - client options
     */
    connect: (relay: RelayOptions, options?: ClientOptions) => Promise<void>;
    /**
     * Disconnect from the Fluence network
     */
    disconnect(): Promise<void>;
    /**
     * Handle connection state changes. Immediately returns current connection state
     */
    onConnectionStateChange(handler: (state: ConnectionState) => void): ConnectionState;
    /**
     * Return peers secret key as byte array.
     */
    getPeerSecretKey(): Uint8Array;
    /**
     * Return peers public key as a base58 string (multihash/CIDv0).
     */
    getPeerId(): string;
}

And last but not least: there is now a function for additional clients in the same page \ nodejs process:

import { dangerouslyCreateClient } from '@fluencelabs/js-client.api';

const client = await dangerouslyCreateClient();

@coder11 coder11 requested a review from shamsartem February 16, 2023 08:35
@shamsartem
Copy link
Contributor

shamsartem commented Feb 16, 2023

@coder11 @folex @boneyard93501 @alari @evgenyponomarev @meph
I don't quite understand the need for such a particular change. I think it was a good mental model to create a peer. If we are going to break stuff I would suggest something like this:

import { createClient } from '@fluencelabs/js-client.api';

// If it's possible I would like not to have a separate getClient method and get the client by default
const client = await createClient({
   // ALL the options are inside one argument object and ALL options should be optional 
   // with random `kras` address being used by default
   // the new property could be
   autoConnect: true // which is true by default
   // also we can add a property that I think would be used a lot
   relays: 'kras' | 'testnet' | 'stage'  | Array<MultiaddrString | Multiaddr>
   // like in fluence cli this would select random peer from network or from array of multiaddrs
})


// I would like not to force users to write switch statements.
// do users really need 'connecting' and 'disconnecting' states?
// I think just having promises would be enough. While promise is not resolved you know - it's connecting
// when it is resolved - you know it's connected

// but if some other, different states are anticipated in the future I would rather have separate
client.onSomeState(() => {
 // do something
})

export interface IFluenceClient {
    /**
     * Connect to the Fluence network
     */
    connect(): Promise<void>;
    /**
     * Disconnect from the Fluence network
     */
    disconnect(): Promise<void>;
    /**
     * Peers secret key as byte array.
     */
    secretKey: Uint8Array; // does it have to be a method instead of just being a property? why Uint8Array is default?
    /**
     * Peers public key as a base58 string (multihash/CIDv0).
     */
    id: string; // the same question here and why base58 string 
}

// also I don't understand dangerouslyCreateClient
// I don't know what's the danger, it's not clear. We should strive not to have any danger in our api
// I don't really like such kind of naming when you make something available but also discourage using it at the same time. If it's possible I would avoid it

@folex
Copy link
Contributor

folex commented Feb 16, 2023

Agree with @shamsartem on dangerouslyCreateClient, sounds worrying. Just createClient would do, imo.

@folex
Copy link
Contributor

folex commented Feb 16, 2023

I think let's rename dangerouslyCreateClient to createClient, and address onSomeState idea later.

I think we need to reconsider the whole JS Client API with Fluence CLI and weak-coupled Aqua API in mind. At the moment, we don't have time for good design session, so let's postpone it and use what Pavel has already implemented.

Except dangerouslyCreateClient.

@shamsartem
Copy link
Contributor

I would not approve what is currently implemented. Sorry I just don't like it, I hope something similar to my solution would be really fast to implement and we can stop on that instead of breaking stuff in order to break it again when we have more time to think

@coder11
Copy link
Contributor Author

coder11 commented Feb 16, 2023

We had a discussion together and decided to make some changes:

  • rename dangerouslyCreateClient to createClient
  • add a little note stating that createClient and Fluence.getClient are low-level api

@coder11 coder11 requested review from DieMyst and folex February 16, 2023 11:37
@coder11 coder11 merged commit 9daaf41 into master Feb 16, 2023
@coder11 coder11 deleted the finalizing-stuff branch February 16, 2023 11:38
@fluencebot fluencebot mentioned this pull request Feb 16, 2023
@shamsartem
Copy link
Contributor

shamsartem commented Feb 16, 2023

@coder11 After Denver would be cool to think about something similar to this to remove implicitness

// in some js file/module if top-level await is working
import { createClient } from '@fluencelabs/js-client.api';
import { aquaFunction, aquaService } from './compiledAqua.js'
export const client = await createClient({
    register: [
      aquaService({
         someMethod() {
            return 1
         }
      }),
      // this call registers function on client. arg is optional and all options are optional (as in current implementation)
      aquaFunction({ ttl: 5000 }),
    ]
})

// in another js file/module
import { client } from './fluenceClient.js'
const result = await client.functions.aquaFunction()

// for vanilla js without ESM and if we wanna have it global
createClient().then((fl) => {
    window.fl = fl
})

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants