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

Politeiagui code refactoring #990

Closed
fernandoabolafio opened this issue Jan 15, 2019 · 13 comments
Closed

Politeiagui code refactoring #990

fernandoabolafio opened this issue Jan 15, 2019 · 13 comments

Comments

@fernandoabolafio
Copy link
Member

As we approach the redesign of Politeiagui, we are considering a significant code refactoring to deal with DX and scalability issues that have been raised by the developers along the development process.
That way we could align a brand new design with better code and performance. See this discussion on Matrix for instance.

Issues raised so far:

  • State Management: The app state management is getting way too complicated and hard to maintain. The use of Redux all over the place is resulting in a lot of boilerplate and edge cases which are hard to understand.
  • Caching: Even though we should be preventing duplicate requests by using Redux, it isn't happening. Leading to higher data consumption.
  • Snew dependency: Most of the code depends on the snew-classic-ui which is a React UI lib version of the old Reddit design. This dependency is hard to maintain and it's components weren't designed for responsivity across multiple screen sizes.

We need to discuss possible solutions to it and evaluate the feasibility of doing it.

Please, write your proposed solutions or thoughts in the comments.

@fernandoabolafio
Copy link
Member Author

fernandoabolafio commented Jan 15, 2019

Proposed solution #1

Layer GraphQL on top of the REST API

Create a layer between www and the GUI client where we could add many data sources to a graphQL server which would be then consumed by the GUI.
Something like this:

data sources (www, dcrdata) -> graphQL server -> graphQL client (Politeiagui)

A good example of how this works can be found here
Some of the benefits would be:

  • Less complexity in the GUI code -> faster development and better UX
  • Less boilerplate code -> smaller bundle size, leading to faster page loading
  • Caching optimization -> less server requests

I propose we develop a small POC which executes a few functionalities of the GUI to evaluate the result and its potential to become mainstream.

The POC:

POC Diff
The designed POC introduces a graphQL server, and it's integration into a graphQL client which is used to display a list of vetted and unvetted proposals.

GraphQL Server

  • It uses the apollo-server package to intermediate the communication between the REST API (politeiawww) and the graphQL client.
  • Schemas and the server setup can be found here.
  • The Politeiawww data source defines how the data is fetched from the REST API.

GraphQL Client

  • The integration with the GraphQL client is straight forward, it defines an apollo-client which is then passed as prop to the ApolloProvider:
/**
 * Apollo Client setup
 */
const link = createHttpLink({
  uri: "/graphql",
  credentials: "include"
});

const client = new ApolloClient({
  link
});

export default class App extends Component {
  render() {
    return (
      <Provider store={store}>
        <ApolloProvider client={client}>
          <Router>
            <LoaderComponent>
              <StagingAlert />
              <SessionExpiresIndicator />
              <HeaderAlertComponent />
              <Subreddit>
                <Routes />
              </Subreddit>
            </LoaderComponent>
          </Router>
        </ApolloProvider>
      </Provider>
    );
  }
}

Full source code can be found here.

  • To demonstrate the feasibility, the POC client displays a list of vetted and a list of unvetted proposals, both replacing the old lists at the index (/) and the admin (/admin) routes respectively. Both lists use the same component for displaying the list. The only difference is the graphQL query used by each of them:
const AllVettedQuery = gql`
  query getAllVetted {
    proposals: vettedProposals {
      name
      username
      censorshiprecord {
        token
      }
    }
  }
`;

const AllUnvettedQuery = gql`
  query getAllUnvetted {
    proposals: unvettedProposals {
      name
      censorshiprecord {
        token
      }
    }
  }
`;

export const AllVetted = graphql(AllVettedQuery)(ProposalList);
export const AllUnvetted = graphql(AllUnvettedQuery)(ProposalList);

Full source code can be found here
ps.: notice that for the vetted proposals the client is requesting name, username, and token while for the unvetted ones only name and token is requested.

Running the POC:

  • Start the graphql server
    go to graphql-server and run:
    yarn && node server.js
  • Start the GUI as usual:
    at the root path of the project, run:
    yarn && yarn start

Resources:
https://blog.apollographql.com/layering-graphql-on-top-of-rest-569c915083ad
https://www.apollographql.com/docs/apollo-server/
https://www.apollographql.com/docs/react/
https://blog.apollographql.com/authorization-in-graphql-452b1c402a9

@sndurkin @tiagoalvesdulce @thi4go @camus-code @lukebp @marcopeereboom please take a look and let me know what are your thoughts.

@victorgcramos
Copy link
Member

Apollo looks like a good solution!

Also, what about the snew-classic-ui? I think it would be better for us to sit down an discuss the whole new design for Politeia, since our UI kit is outdated. Also, we can work on improving the mobile responsivity, which we don't have.

IMO, creating a new design kit from scratch would take less time than keep improving an existing one.

I'm able to help to create this!

@fernandoabolafio
Copy link
Member Author

IMO snew-classic-ui has to be dropped and a new UI lib has to be built taking in consideration responsiveness, customizability and general UX.

Also, what about the snew-classic-ui? I think it would be better for us to sit down an discuss the whole new design for Politeia, since our UI kit is outdated. Also, we can work on improving the mobile responsivity, which we don't have.

@crypto-rizzo
Copy link
Contributor

Completely agree @fernandoabolafio.

snew-classic has become a big bottleneck and I don't really think it adds that much in terms of functionality. Not to mention it prevents people from easily being able to contribute to the GUI repo.

I'm not sure if the ideas is to create a UI lib from scratch or to implement an existing one, but I've come to really enjoy Semantic UI - React for building responsive & customizable UX. They have a pretty expansive module / element collection that covers pretty much everything we already currently use within the GUI.

@victorgcramos
Copy link
Member

@camus-code this UI Kit is awesome! Great idea!

@orthomind
Copy link
Contributor

orthomind commented Feb 11, 2019

Just as an outsider to politeia code looking in, as a suggested alternative to Apollo Server, I've had good experiences myself using Apollo Client with the Go based gqlgen as the GraphQL server.

It's something I may have time to help out with in terms of coding as well, if you go that way.

Overall, I've found GraphQL brings great benefit to the front end, by pushing a lot of that complexity to the server. It's harder to optimise on the server, but optimisations can come later. Apollo Client also has caching options to reduce calls to the server which can help out.

@fernandoabolafio
Copy link
Member Author

@orthomind that's an interesting point of yours. If we opt to use a graphQL server written in go, we may want to have the gql service within the politeia repo where all the go code lives in. So, every functionality that we want to migrate from REST to gql would require 2 PR's, one for Politeia and another one for Politeiagui.
I particularly think the gql server would work better closer to the client code. So devs working on politeiagui could easily add new functionalities by using the existent REST endpoints and write the gql bits and the UI bits in a single PR.
I am looking forward to hearing your thoughts about it.

@orthomind
Copy link
Contributor

Take my remarks with a grain of salt, as I'm not much familiar yet with how politeia works and the various components that make it up. With that now said...

  • I think it makes sense to include any such gql server as a separate project. If it sits as a layer between, say, politeia back end(+other back-ends) and the frontend, its job is to translate between both these sides, and doesn't obviously fit with one more than the other. It doesn't belong purely in politeia because it might plausibly pull data from more than just politeia. It doesn't belong purely in politeiagui because other front end services might one day want to interact with it as well (e.g., native mobile apps, alternative websites).
  • When code is split across multiple repositories, it certainly does get more cumbersome to release changes that require updates in all repos. So you are right about that particular additional burden. Could the project be included in politeiagui initially even if it's based on Go, and split into a separate project once it has matured enough?
  • I personally always have a preference for Go over JS for server-side tools (here I reveal my bias!).

@fernandoabolafio
Copy link
Member Author

fernandoabolafio commented Feb 25, 2019

Sorry for the delay @orthomind and I much appreciate your comments. I think your point that it may belong neither in politeiagui or in politeia makes a lot of sense. It can be a separate service which politeiagui or any other frontend consumes from. Not sure about it should be born inside politeiagui or not. If it's written with nodejs, I would rather keep it in the same repository.

I think it makes sense to include any such gql server as a separate project. If it sits as a layer between, say, politeia back end(+other back-ends) and the frontend, its job is to translate between both these sides, and doesn't obviously fit with one more than the other. It doesn't belong purely in politeia because it might plausibly pull data from more than just politeia. It doesn't belong purely in politeiagui because other front end services might one day want to interact with it as well (e.g., native mobile apps, alternative websites).

About writing in Golang may be a good idea since we have Go specialist in the team. Although my first intention is to make the graphQL layer friendly for the frontend developers so they can quickly abstract services from the backend and put it to run in politeiagui.

I personally always have a preference for Go over JS for server-side tools (here I reveal my bias!).

@thi4go
Copy link
Member

thi4go commented Feb 26, 2019

Hey guys. Been thinking about this GraphQL approach to our refactor, and I believe it will bring many benefits to the frontend developers, current ones and newcomers. Currently, our redux layer is doing a lot of heavy lifting, resulting in bloated boilerplate whenever we want do add a feature, be it a simple or complex one. Adding this aditional GraphQL layer would reduce complexity by a good margin.

I've been trying to think what would change practically in our code base, and from what I understood (correct me if I'm wrong please) graphQL queries can substitute both connectors and selectors. Each component would still need a provider (or whatever its name) to pass down its necessary props, which would be fed by custom queries from apollo client to that component. So in @fernandoabolafio's PoC, in componets_v2, Proposals.js serves as this provider, while ProposalsList.js is the actual component that consumes the data. Currently, we reutilize some of the connectors throughout different components, and although this might be a good code reuse practice, sometimes it results in having injected props that are'nt used, or having to add new props to the connector that only one component will use. So from my understanding each component would then have its own custom connector/provider passing down data by graphQL queries. This would make the development of components a lot easier, by not having to go through all that redux boilerplate, and instead just hacking in the provider that is handling queries and local state, and lives next to the component.

Also thinking about local state, I stumbled upon apollo-link-state in the documentation of the tool. It serves to manage both remote data and local state data in apollo client. It extends schemas defined in apollo server inside the client with additional structures that correspond to local state data, unifying all of our data needs in one location, being our single source of truth. Aside of a simplified state management, we'd get a smart cacheing out of the box, as it was mentioned here in this thread.

I'm hacking into the PoC to see if I can advance in some parts, especially in a satisfying architecture for the new components folder, the substitution of the redux folders boilerplate, and how we'd manage local state data within apollo client. Have you guys thought of other ways we can advance in this PoC? Any input would be great. @fernandoabolafio @victorgcramos @camus-code @tiagoalvesdulce @orthomind @sndurkin

@fernandoabolafio
Copy link
Member Author

I've been thinking more about it and let's keep this possibility in mind but I think we need to discuss this after the new design implementation and also the decoupling of snew. After those two things are done, it will be much more clear for use if we can just improve our Redux code or we need to switch to graphQL.

@go1dfish
Copy link
Contributor

go1dfish commented May 1, 2019

Yeah people didn't seem to care as much about the reddit like heritage of the design as much as I thought they would; so no hard feelings cutting it out of pgui.

I'd like to suggest that React hooks and Context Providers are excellent, make redux completely unnecessary; and would be a great way to make the redux code more manageable without having to also dive into graqhql at the same time.

It's similar enough to the redux pattern and selectors that it could be done piecemeal on a per-component basis.

@fernandoabolafio
Copy link
Member Author

Most of it was accomplished with the redesigned version. Other parts of it such as state management and caching optimization are being tracked under other issues.

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

No branches or pull requests

6 participants