Skip to content

Instantly share code, notes, and snippets.

@tomusdrw
Created June 2, 2021 14:16
Show Gist options
  • Save tomusdrw/6be7adb067ba44920931d8146f7467f7 to your computer and use it in GitHub Desktop.
Save tomusdrw/6be7adb067ba44920931d8146f7467f7 to your computer and use it in GitHub Desktop.
type PayloadType = {
};
type EstimateFeeType = {};
class ApiCalls {
async estimateFee(chain: Chain, laneId: LaneId, payload: PayloadType): EstimateFeeType {
const payloadType = createType(chain, "OutboundPayload", payload);
return await stateCall(estimateFeeMethod, ...);
}
}
function Component() {
const apiCalls: ApiCalls = useApiCalls();
const { isFeeCalculating, setFeeCalculating} = useState(false);
const { fee, setFee } = useState(null);
const { feeError, setFeeError } = useState(null);
useEffect(() => {
setFeeCalculating(true);
setFee(null);
try {
const fee = await apiCalls.estimateFee(chain, laneId, payload)
setFee(fee);
setFeeCalculating(false);
} catch (e) {
setFeeCalculating(false);
setFeeError("error");
}
});
}
import {useEstimateFee} from './hooks/api/useEstimateFee';
function Component() {
const { fee: number | null, feeError: string|null, isFeeCalculating: bool, recalculateFee: () => void } = useEstimateFee(laneId, ...);
fee==null
isFeeCalculating == true
//
useEffect(() => {
recalculateFee();
});
}
@tomusdrw
Copy link
Author

tomusdrw commented Jun 7, 2021

Cool stuff @wirednkod, thanks for the write up.

I really like the idea with getStateCall/useStateCall functions. Take a look below, left a few comments and also made the useStateCall generic so that we don't have to duplicate that code for every API call. Obviously this will require a bit of TS type magic to get work.

// ./src/hooks/api/useStateCalls.ts
import { useState, useCallback } from 'react';

export const getStateCall = async (chain, data, at): Codec => {
  const { getValuesByChain } = useChainGetters();
  const getChainValues = useCallback((chain) => getValuesByChain(chain), [getValuesByChain]);
  
  const {
    api,
    substrateValues: { estimatedFeeMethodName }
  } = getChainValues(chain);

  const params = [estimatedFeeMethodName, data];
  if (at) { params.push(at); }
  const response: Codec = await api.rpc.state.call<Codec>(...params); 
  // Add any transformation from api needed here
  return response;

  // [ToDr]  the extra `try-catch` is not necessary, errors are propagated by default.
};

// [ToDr] This function can be easily made generic to avoid duplication
export const useGenericCall = (getStateCall) => {
  const [isLoading, setIsLoading] = useMountedState<boolean>(false);
  const [error, setError] = useMountedState<unknown>(null);
  const [data, setData] = useMountedState<unknown>(null);
  
  // [ToDr] you need to always use `useCallback`, also worth to pass all the deps (note that indeed they will never change in this particular example, but better to be safe than sorry).
  const execute = useCallback(async (params...) => {
    try {
      setIsLoading(true);
      const stateCall = await getStateCall(...params);
      setData(stateCall);
      return stateCall;
    } catch (e) {
      setError(e);
      setIsLoading(false);
      // throw e; /// this is unnecessary and will trigger extra errors on the console - we did handle the exception already.
    }
  }, [setIsLoading, setData, setError, setIsLoading, getStateCall]);
  
  return {
    isLoading,
    error,
    data,
    execute,
  };
};

Regarding state management - I like your approach, there are few things that I'd like to point out though:

  1. The global state does not need to be a reducer - it's fine to have top-level useState functions passed via context as well. useState/reducer discussion is a bit out of scope though, so I'm just noting it down - don't want to trigger anyone :)
  2. There is one more thing worth noting - components that don't use neither local nor global state at all - stateless components which simply get props externally. IMHO in current app and in your design they are not appreciated enough, but imho they are awesome and make it extremely easily to reason about the application. In my ideal view, some of the components should simply be responsible only for presentation logic and don't use any of domain-specific hooks at all (maybe some local useState for presentation state (like isToggled) sometimes). IMHO the more complex the display/component is - the more the component should avoid using domain-specific hooks.

@hbulgarini

@tomusdrw I'm not sure if i understand completely what you are trying to say here. Are you saying that introducing action creators in the way i proposed add complexity because of we would be adding dependent reducers between each other? i'm not following you when you specifically said: I'm not sure if they add that much value if they are mixed with hooks anyway ... hooks in my proposal are mainly used for exposing the global variables and build abstraction layers for the component that will use those global values.

My point is that we have both useState and useReducer for global state management and I think it's without any particular reason - like if I was to add a new global state I wouldn't know if it should be a reducer or useState.

For your code i have the exact question as the i did for nikos in my reply: how are you sharing values into a global state?

You either pass them via props or you pass them via context. I don't really know if there is any 3rd way of doing that. Examples:

function App() {
  const { apiConnection, setApiConnection } = useState(null);
  return <>
      <ReadOnlySubComponent api={apiConnection} />
      <ConnectingComponent setApiConnection={setApiConnection} />
  </>;
}

or

function App() {
  const { apiConnection, setApiConnection } = useState(null);
  return <>
      <ApiProvider connection={apiConnection}>
        <ReadOnlySubComponent />
      </ApiProvider>
     <SetApiProvider setConnection={setApiConnection}> 
        <ConnectingComponent />
     </SetApiProvider>
  </>;
}

function ReadOnlySubComponent() {
   const { connection } = useContext(ApiContext);
   ...
}

The same design can be applied not only in the top-level component, but anywhere down the hierarchy.
Obviously passing props is super cumbersome, but it simplifies thinking about the code a lot - cause you see all of the things that are being used right from reading the prop definition.
Context are more magical, hence they should be use with great care - i.e. you don't want a random tiny component (for instance a button) suddenly making a global state call if that's something that could be encapsulated in a parent component and the function to invoke could be passed as a prop.
Hiding context usage and a lot of hook-nesting (i.e. hook calling another hook, etc) make it difficult to follow the logic of the application, of course it's needed to avoid duplication, but the hardest part in programming is navigating between too much and too little of it (the hardest except for naming things, obviously:))

@wirednkod
Copy link

wirednkod commented Jun 7, 2021

@todr @hbulgarini Thank you for your kind words.

Concerning 1) : useReducer is a (lets say) more complicated useState. Its not frowned upon or dismissed for use by me, and it has benefits that can alter complicated states while useState is more of a shallow state holder (P.S. I promise never to argue about reducers again)
Concerning 2) : I do not mention the "presentational" components as its not part of our discussions. I have said before that there must be "clever"/"parent"/"container" components that take care of data, states, api calls (if needed), dispatchers etc etc, and simple "stupid"/"childs"/"presentational" components that the ONLY thing that should do is re-render based on prop and (worst case scenario) maybe fire back something.

Sample code is there just for pointing out one solution for our refactor scenarios, and not of course that would not be the ultimate solution. This is meant just as some food for thought. It has a lot of fixes/makeovers to make the code reusable and better.

@hbulgarini
Copy link

I was diving a little more on this and trying to bring all what we discuss to our existing processes. And here my conclusions:

  1. store.js : is exactly what we have now, but split in different reducers per concern. I would not join them all in one global reducer since one update on each specific concern would force all the reducers to update.

  2. useApi.ts: I think there should be two separate hooks, the useApi.ts as it is in your example code nikos, but then the generic call should be extracted to a different process.

  3. useGenericCall.ts: i think this is a pretty useful and reusable hook but it was pretty hard to add the logic to handle global calls which are provided to it as input props of the hook. The main problem i faced with this is that when you have to provide the global functions as input props for this hook , it is really easy to forget a useCallback that will make the whole app to enter in a rerender infinite loop.
    So my recommendation is to keep this generic call hook, but to only use it when this hook is going to be used locally ( without having to use this values as global)

  4. Global calls: My preferred options is to use reducers. Not only for the benefits of state management flow , etc, but also because i have confirmed that the use of the dispatcher is much much more secure in terms of rerender control than sharing trough props chains or context a wrapped function that will end calling a setState. In case those calls are not properly wrapped in a useCallback everything gets rerender again because the function reference is different in each render.

So my suggested rule is in case a value has to be global ( as estimatedFee and payload ) , they should have their own dedicated hook that will expose a function that will handle their corresponding update status with dispatchers ( similar to my first comment )

@wirednkod
Copy link

wirednkod commented Jun 11, 2021

The following can really make all the above TLDR and show what I mean in idea.ts and project I built.
I am approaching it more like this:

  • Layer 1: API Layer ----------------------> PolkadotJS API
  • Layer 2: Bridges API specific Layer -----> Bridges API that uses specific needed things from polkadotJS API (not all API calls from polkadot JS are needed
  • Layer 3: Bridges Simple/Specific Business logic ----------> Functions/hooks that are using the Bridges API for 1 VERY specific reusable action (get the chain, estimate fee etc etc)
  • Layer 4: Bridges Advanced/Complicated Logic ---------> Functions/hooks that can use any of Layers 2, 3 to make complicated logic (send a transaction and estimate fee and do something else at the same time)
  • Layer 5: Bridges or other Bridges-related UI ------> UI components that can use any of Layers 2 - 4, to re-create or reuse Bridges functionalities (simple or complicated)

Layers 2 -4 is the "library" we need to build. Layer 5 should be the actual Bridges UI. Layer 2-4 should ALWAYS work with specific version of Layer 1

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