aeternity / aepp-sdk-js

JavaScript SDK for the æternity blockchain
http://docs.aeternity.com/aepp-sdk-js/
ISC License
120 stars 59 forks source link

Make possible to create SDK instance in sync #762

Closed davidyuk closed 2 years ago

davidyuk commented 4 years ago

I see two points when it will be useful:

The Base app has a lot of pages that can be opened directly, most of them depend on data provided by sdk. Right now before calling a method of sdk it should ensure that sdk is initialized, and only after that to call the required method. This is an error-prone approach and also it makes us write duplicate logic. The other approach would be is to postpone the app running until sdk is initialized, but it is not acceptable, because a showing of app UI will be postponed until sdk initializes.

To support TypeScript we should be aware of method types in compilation time that is not possible for sdk.api.* endpoints that are build in runtime by swagger file. If these endpoints would be generated before runtime then it would be possible to check type correctness.

nduchak commented 4 years ago

Generating of sdk.api.* is not the one place which make sdk initialization async, the second one is checking for compatibility which require additional request to the node

davidyuk commented 4 years ago

I was guessing than there is not a single async action in initialization.

is checking for compatibility which require additional request to the node

It may be done as a separate method exposed to the user. Also, this compatibility check may be called on the first call of any of sdk.api.* methods.

Is there any other async stuff that currently runs in time of initialization?

nduchak commented 4 years ago

Not sure, need to check it

nduchak commented 4 years ago

@davidyuk What exact data you need to show these pages? If it's data retrieved from Node API anyway you need to wait until SDK initialized?

davidyuk commented 4 years ago

What exact data you need to show these pages?

I think any of the possible requests, for example, the current nonce or to resolve name into name entry.

If it's data retrieved from Node API anyway you need to wait until SDK initialized?

I can fetch it by myself then I don't need to wait for sdk initialization. The thing is can this initialization be done in sync or at least be postponed until the first request to node api? I know that I need to wait for data retrieved via Node, but I don't want to wait twice.

nduchak commented 4 years ago

You can just not waiting until SDK is initialized(I mean doesn't block code execution) and show your pages. Anyway if you want to get something from the node, you need to wait until SDK will be initialized.

davidyuk commented 4 years ago

Anyway if you want to get something from the node, you need to wait until SDK will be initialized.

Is it possible to avoid async initialization of sdk? Currently, I have to ensure that it is initialized on every page that can be opened directly. It would be helpful if sdk won't require async initialization.

nduchak commented 4 years ago

It's not possible

davidyuk commented 4 years ago

The last time you decided to check it, did you find something more? Can you share with me why it is not possible?

nduchak commented 4 years ago

Anyway if you want to get something from the node, you need to wait until SDK will be initialized.

Is it possible to avoid async initialization of SDK? Currently, I have to ensure that it is initialized on every page that can be opened directly. It would be helpful if SDK won't require async initialization.

I don't see any reason to spend time on it if your page needs SDK instance then you need to wait anyway. If your page does not actually need SDK instance but you prefer to have it then please skip the Promise resolution on your side. Can you be more specific and provide an example of what you try to achieve please?

davidyuk commented 4 years ago

I don't see any reason to spend time on it

This fix should increase usability in the case that I have explained above, don't sdk team interested in it?

if your page needs SDK instance then you need to wait anyway

I need to wait, but I don't want to wait two times if it can be avoided.

skip the Promise resolution on your side.

What does it mean and how it is supposed to work?

davidyuk commented 4 years ago

Can you be more specific and provide an example of what you try to achieve please?

Sure, let's assume that I have a file that creates an sdk instance:

import { Universal } from '@aeternity/aepp-sdk';

export default Universal({ ... });

and several pages using it:

import sdk from '../sdk.js';

function async render() {
  const sdkInstance = await sdk;
  return await sdk.api[<some method name, depends on particular page>](...);
}

Writing await sdk in every page that uses sdk is tediously and I would like to avoid that and do like this:

import sdk from '../sdk.js';

function async render() {
  return await sdk.api[<some method name, depends on particular page>](...);
}
nduchak commented 4 years ago

Thanks for examples: So you can simply prepare an SDK singleton class

import SdkSingleton from '';

export default SdkSingleton;

Then in some phase before rendering your components(maybe some router root hook?) just import SDK singleton and initialize it new SdkSingleton()

You can avoid this Promise resolving in each render method by resolving it ones.

davidyuk commented 4 years ago

I'm aware of this, check the first message:

The other approach would be is to postpone the app running until sdk is initialized, but it is not acceptable, because a showing of app UI will be postponed until sdk initializes.

Or maybe I have misunderstood something. That is the difference between SdkSingleton and usual SDK stamps?

davidyuk commented 4 years ago
import { Universal } from '@aeternity/aepp-sdk';

let sdk;

export const init = async () => {
  sdk = await Universal({ ... });
}

export default sdk;

Root page/route:

import { init } from '../sdk.js';

function async someInitMethod() {
  await init();
  ...
}

Usual page:

import sdk from '../sdk.js';

function async render() {
  return await sdk.api[<some method name, depends on particular page>](...);
}

Is it the same as your approach?

nduchak commented 4 years ago

Looks like the same, but you don’t need to await it in root page. You need to show all part of UI which is independent from data from sdk and have some interface to get the sdk instance if it’s already resolved

davidyuk commented 4 years ago

have some interface to get the sdk instance if it’s already resolved

This part is unclear to me. Do you mean to make a wrapper object around sdk? Like this one:

import { Universal } from '@aeternity/aepp-sdk';

const sdkPromise = Universal({ ... });

const genMethodWrapper = (methodName) => async (...args) => {
  const sdk = await sdkPromise;
  return sdk[methodName](...args);
}

export default {
  method1: genMethodWrapper('method1'),
  // ...
};

Actually, I don't like this approach because we will be had to maintain a list of sdk methods on our side. It can be avoided in this way:

import { Universal } from '@aeternity/aepp-sdk';

const sdkPromise = Universal({ ... });

export default {
  call(methodName, ...args) {
    const sdk = await sdkPromise;
    return sdk[methodName](...args);
  },
};

But we will have to rewrite all sdk-related calls and in general it looks worse.

One more way is to use modern Proxy and Reflect objects:

import { Universal } from '@aeternity/aepp-sdk';

const sdkPromise = Universal({ ... });

export default new Proxy({}, {
 get(target, prop) {
    return async (...args) => {
      const sdk = await sdkPromise;
      return sdk[prop](...args);
    };
  },
  // ...
});

the disadvantage is this method is a lack of support in IE 11: https://caniuse.com/#feat=proxy. And also I'm not sure how to handle static properties of sdk instance.

As you can see, solving this problem requires some not obvious engineering to make on aepp side. It would be better to solve this problem somehow on the sdk side if it is possible.

nduchak commented 4 years ago
// getSDK
export default (() => {
  let instance
  return async () => {
    if (!instance) {
      instance = Universal({})
      return instance
    }
    return instance
  }
})()

Then in root component

getSDK() // do not wait for sdk intance creation

Then in any component

const sdkIns = await getSDK() // Here sdk already created

@davidyuk What do you think?

davidyuk commented 4 years ago

In this case, I still have to write await two times to call a method, like:

import getSdk from './sdk';

export default {
  methods: {
    methodA() {
      const sdkIns = await getSDK();
      await sdkIns.api[<some method name>](...);
    },
    methodB() {
      const sdkIns = await getSDK();
      await sdkIns.api[<some method name>](...);
    },
    methodC() {
      const sdkIns = await getSDK();
      await sdkIns.api[<some method name>](...);
    },
  },
}

This can be avoided by making sdk to init in sync or by making some wrapper around sdk as I proposed above.

nduchak commented 4 years ago
// getSDK
export default (() => {
  let instance
  return async () => {
    if (!instance) {
      instance = Universal({})
      return instance
    }
    return instance
  }
})()

Then in root component

getSDK() // do not wait for sdk intance creation

Then in any component

const sdkIns = await getSDK() // Here sdk already created

@davidyuk What do you think?

In this case, you have just one SDK instance which is asynchronously initialized in your root component. And then when you make await getSDK() it's doesn't make any requests and get already initialized sdk instance(so you don't wait for that)

davidyuk commented 4 years ago

so you don't wait for that

The goal is not to avoid waiting, but not writing await two times anytime when I need to request something through SDK. I was talking about it above.

Also, your solution works the same as the example of a problem that I wrote by your request: https://github.com/aeternity/aepp-sdk-js/issues/762#issuecomment-558134678 the imported promise of sdk in my example will wok the same as your getSDK()

nduchak commented 4 years ago

Ok, now i get. But what wrong with writing await? You resolve the promise which already resolved(any requests sent)?

davidyuk commented 4 years ago

I just don't want to write it, it seems redundant to me. I see that double await doesn't lead to additional requests.

In our specific use case, we have made a vuex plugin that puts sdk promise and then the actual sdk instance to some global vuex variable. It allows us to use sdk without awaiting (this makes code cleaner) until cases like when sdk is not initialized yet (when some page is opened directly). These cases brokes the whole concept and I have to write sdk.then ? await sdk : sdk in random places. This is why I'm asking you to fix this on sdk side. I think most of dapp developers will be grateful for doing this.

thepiwo commented 4 years ago

I agree with @davidyuk that we should improve some things here, we should find a way to make it more straight forward for new devs to build a well-performing aepp. Maybe some singleton helper from the SDK could be of use here.

But also I agree with @nduchak, with the aepps we build right now we found our own way similar to this to https://github.com/aeternity/aepp-sdk-js/issues/762#issuecomment-559082836 to work around this issue. This was not hard for us to engineer and we gain full control over the solution, often we include some more helper functions in our singleton instance implementation.

davidyuk commented 4 years ago

This was not hard for us to engineer and we gain full control over the solution, often we include some more helper functions in our singleton instance implementation.

I meant that it is difficult to build a solution that won't make the developer write await twice without making a wrapper for each SDK's method. Can you point to helper functions that you have defined in your singleton instance?

kenodressel commented 4 years ago

Our setup is similar to what @nduchak mentioned in his suggestion here https://github.com/aeternity/aepp-sdk-js/issues/762#issuecomment-559180218. Since we use the SDK in so many places we initiate and load the SDK once with await in the wrapper component and simply use the functions in the child components. This of course is not really recommended as it could theoretically happen that we try to access the SDK without it being ready. In practice this has never been an issue due to the rather quick initialization time of the SDK compared to the whole rendering process.

The util itself is not really of interest here as it only handles the initialization of multiple wallets and the preferred wallet connection. You can look up the relevant code lines here:

Initialization in the wrapper component: https://github.com/aeternity/aepp-governance/blob/master/governance-aepp/src/App.vue#L83

Initialization in the util: https://github.com/aeternity/aepp-governance/blob/master/governance-aepp/src/utils/aeternity.js#L46

Example usage: https://github.com/aeternity/aepp-governance/blob/master/governance-aepp/src/views/Poll.vue#L294

Again this is not a solution for everyone but it has been working fine for us.

davidyuk commented 4 years ago

This of course is not really recommended as it could theoretically happen that we try to access the SDK without it being ready. In practice this has never been an issue due to the rather quick initialization time of the SDK compared to the whole rendering process.

This is actually happening to us, we need to find a different approach, and I would like to avoid writing of an additional await sdk in every entry point.