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

Suggestions on improving the codebase architecture #42

Open
yinkakun opened this issue Jul 5, 2022 · 1 comment
Open

Suggestions on improving the codebase architecture #42

yinkakun opened this issue Jul 5, 2022 · 1 comment

Comments

@yinkakun
Copy link
Contributor

yinkakun commented Jul 5, 2022

It'll also allow for customization of Input components, for example, this input won't allow alphabets to be typed (alphabets can be typed in the current input), but Antd Input components doesn't allow most of these props.

<Input
      // universal input options
      inputMode="decimal"
      autoComplete="off"
      autoCorrect="off"
      // text-specific options
      type="text"
      pattern="^[0-9]*[.,]?[0-9]*$"
      placeholder={placeholder || '0.0'}
      minLength={1}
      maxLength={79}
  • I think it'll be good for components like Input and Dropdown components to be stateless, they can then be used in TradeInteraction, and MintInteraction each passing its data to it, that'll reduce the multiple uses of useEffect in the code base.

So TradeInteraction will look like:

const TradeInteraction = () => { 
// hooks
return (
<div>
  <Row>
    <CurrencyInput />
    <TokenSelectorDropdown />
  </Row>
  <SwapArrow />
  <Row>
    <CurrencyInput />
    <TokenSelectorDropdown />
  </Row>
</div>)
}

In contrast to

const TradeInteraction = () => { 
  return (
    <SingleInteraction
     // props
    />
  );
}
  • Add a Prettier config to the codebase - for consistence formatting throughout the codebase

Redux

  • It is recommended to use a global typesafe instance of useSelector and useDispatch. That way, the types of dispatch and selectors can be directly inferred from the store (and not need to be manually passed in every time)
import { TypedUseSelectorHook, useDispatch, useSelector } from 'react-redux';

const store = // store setup.

type AppState = ReturnType<typeof store.getState>;
type AppDispatch = typeof store.dispatch;

export const useAppDispatch = () => useDispatch<AppDispatch>();
export const useAppSelector: TypedUseSelectorHook<AppState> = useSelector;

export default store
  • I wonder why this action is dispatched every 2 seconds.
    - https://redux.js.org/style-guide/#avoid-dispatching-many-actions-sequentially
    > While UI updates queued from React event handlers will usually be batched into a single React render pass, updates queued outside of those event handlers are not. This includes dispatches from most async functions, timeout callbacks, and non-React code. In those situations, each dispatch will result in a complete synchronous React render pass before the dispatch is done, which will decrease performance.

https://github.com/indexed-finance/indexed-interface/blob/b4a6c09d534f523db279f201154fab2f89e2205a/src/features/middleware.ts#L234

      setTimeout(() => {
        dispatch(actions.finishedFetchingOffChainCalls(offChainCalls));
      }, 2000);
  • Handle pending and rejected actions created by createAsyncThunks in the reducer, currently only the fulfilled status is mostly handled

Folder structure

  • The Routes / Pages components folder should be a direct child of the src folder to match popular patterns like nextjs folder structure, and I think it's more intuitive instead of it being nested in the components folder.

  • Re-evaluate the atomic folder structure, having read this reddit post and comments and from my personal experience on this project, I think it'll be more intuitive to group related components together in a folder, instead of grouping by Atomic design rule, which not everyone is familiar with.

Server State Management

Looking more into the 'Assess frontend infra' task details, one of the needs is: "We need to configure the helper’s time-series functions to return the latest available data source."

So I suggest we use a library that's specialized in server state management like React Query - https://tanstack.com/query/v4/docs/overview instead of Redux, as I noticed Redux is mostly used for managing Server states.

Miscellaneous

  • The Readme should be updated with a guide on how to get the development server running.

  • Consider using vite for the React environment. - vite is much faster, should also probably fix the need to increase nodejs heap memory, also while working on a certain bug and wanted to track component re-renders with why-did-you-render, it wasn't successful because CRA ^4 does use the automatic JSX transformation even with react-app-rewired

  • URL State for Trade widgets - the Buy, Mint, and Burn active tab state should be a URL state so when the URL is shared, it maintains its active tab.

  • Consider using absolute imports instead of relative imports - Because it makes it easier to move files around and avoid messy import paths i.e @/components/MyComponents over ../../components/MyComponents

@deomaius
Copy link
Contributor

Given the foundational work in the refactoring done in the drain reversal application, it is clear styled-components is a minimal route with extensive composability to nurturing design. Additionally it is lightweight in comparision to bloated component frameworks, which import a huge amount of unutilised files.

State management is also in need of an entire rework, as stated above there is countless dispatches being irresponsibly repeated when there is no need to be, causing massive memory leaks to the application. There is good foundational support to move forward, scoping the possibility of modularity with the store should also be a front facing priority with the inveitable alternative network deployments of the protocol.

With the regulatory climate as of late, there must be efforts to accelerate in censorship resistant access to the frontend. IPFS is a beautiful and simple resource to tap into such a paradigm, and by deafult should be the enviroment to host the repository so that no central trust is put in a hosting provider and that the build is easily verifable.

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

No branches or pull requests

2 participants