{"action":"created","issue":{"url":"https://api.github.com/repos/strvcom/dep-manager-web/issues/12","repository_url":"https://api.github.com/repos/strvcom/dep-manager-web","labels_url":"https://api.github.com/repos/strvcom/dep-manager-web/issues/12/labels{/name}","comments_url":"https://api.github.com/repos/strvcom/dep-manager-web/issues/12/comments","events_url":"https://api.github.com/repos/strvcom/dep-manager-web/issues/12/events","html_url":"https://github.com/strvcom/dep-manager-web/issues/12","id":331472522,"node_id":"MDU6SXNzdWUzMzE0NzI1MjI=","number":12,"title":"Code review 12.6.2018","user":{"login":"petyrek","id":10710661,"node_id":"MDQ6VXNlcjEwNzEwNjYx","avatar_url":"https://avatars2.githubusercontent.com/u/10710661?v=4","gravatar_id":"","url":"https://api.github.com/users/petyrek","html_url":"https://github.com/petyrek","followers_url":"https://api.github.com/users/petyrek/followers","following_url":"https://api.github.com/users/petyrek/following{/other_user}","gists_url":"https://api.github.com/users/petyrek/gists{/gist_id}","starred_url":"https://api.github.com/users/petyrek/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petyrek/subscriptions","organizations_url":"https://api.github.com/users/petyrek/orgs","repos_url":"https://api.github.com/users/petyrek/repos","events_url":"https://api.github.com/users/petyrek/events{/privacy}","received_events_url":"https://api.github.com/users/petyrek/received_events","type":"User","site_admin":false},"labels":[{"id":1135412130,"node_id":"MDU6TGFiZWwxMTM1NDEyMTMw","url":"https://api.github.com/repos/strvcom/dep-manager-web/labels/Difficulty:%20challenging","name":"Difficulty: challenging","color":"db3f47","default":false},{"id":1135414397,"node_id":"MDU6TGFiZWwxMTM1NDE0Mzk3","url":"https://api.github.com/repos/strvcom/dep-manager-web/labels/Type:%20Infrastructure","name":"Type: Infrastructure","color":"86c0d8","default":false},{"id":1135410538,"node_id":"MDU6TGFiZWwxMTM1NDEwNTM4","url":"https://api.github.com/repos/strvcom/dep-manager-web/labels/Type:%20breaking%20change%20%E2%9A%A0%EF%B8%8F","name":"Type: breaking change ⚠️","color":"083cd8","default":false}],"state":"open","locked":false,"assignee":{"login":"bsunderhus","id":5483269,"node_id":"MDQ6VXNlcjU0ODMyNjk=","avatar_url":"https://avatars1.githubusercontent.com/u/5483269?v=4","gravatar_id":"","url":"https://api.github.com/users/bsunderhus","html_url":"https://github.com/bsunderhus","followers_url":"https://api.github.com/users/bsunderhus/followers","following_url":"https://api.github.com/users/bsunderhus/following{/other_user}","gists_url":"https://api.github.com/users/bsunderhus/gists{/gist_id}","starred_url":"https://api.github.com/users/bsunderhus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bsunderhus/subscriptions","organizations_url":"https://api.github.com/users/bsunderhus/orgs","repos_url":"https://api.github.com/users/bsunderhus/repos","events_url":"https://api.github.com/users/bsunderhus/events{/privacy}","received_events_url":"https://api.github.com/users/bsunderhus/received_events","type":"User","site_admin":false},"assignees":[{"login":"bsunderhus","id":5483269,"node_id":"MDQ6VXNlcjU0ODMyNjk=","avatar_url":"https://avatars1.githubusercontent.com/u/5483269?v=4","gravatar_id":"","url":"https://api.github.com/users/bsunderhus","html_url":"https://github.com/bsunderhus","followers_url":"https://api.github.com/users/bsunderhus/followers","following_url":"https://api.github.com/users/bsunderhus/following{/other_user}","gists_url":"https://api.github.com/users/bsunderhus/gists{/gist_id}","starred_url":"https://api.github.com/users/bsunderhus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bsunderhus/subscriptions","organizations_url":"https://api.github.com/users/bsunderhus/orgs","repos_url":"https://api.github.com/users/bsunderhus/repos","events_url":"https://api.github.com/users/bsunderhus/events{/privacy}","received_events_url":"https://api.github.com/users/bsunderhus/received_events","type":"User","site_admin":false}],"milestone":null,"comments":0,"created_at":"2018-06-12T07:40:48Z","updated_at":"2018-11-21T12:36:54Z","closed_at":null,"author_association":"NONE","body":"YO YO YO, i did a little review, hope you will find something useful here\r\n\r\n## SVGs\r\nnot minimized\r\nDump the svg code in here https://jakearchibald.github.io/svgomg/ and it will spit out minimized output\r\n\r\n## CSS reset\r\nThis could be installed as dependency and simply included, or it is a part of the `polished` package.\r\nThough there's not necessarily anything with what you're doing. But maybe that people can mess with it this way, when it's not encapsulated, but they really shouldn't.\r\n\r\n## Flow\r\nLots of type errors everywhere, broken imports.\r\nTo fix imports do something like\r\n`/flow-typed/ignore.js`\r\nwith the following contents\r\n```\r\n// @flow\r\n\r\ndeclare module \"react-swipeable-views\" {\r\n  declare module.exports: any\r\n}\r\n```\r\n\r\nInference doesn't work here\r\n`export default class Dashboard extends Component<*, *> {`\r\nas it has too little information.\r\nFull shape of state cannot be infered from\r\n```\r\nstate = {\r\n    projects: {},\r\n    latestLibraries: {},\r\n    projectLibraryRelation: [],\r\n    filteredProjects: [],\r\n    filteredLibraries: [],\r\n    isLoading: true\r\n  };\r\n``` \r\nso Ideally you want to be a little more explicit to ensure type safety.\r\n\r\n## Apollo\r\nWhat you have in `/src/App.js` using the `<Query>` component seems to me like a horrible pattern. Unclear what happens and seems like it could be buggy when this component rerenders, because you are invoking side effects from within the render method. I would be ultra carefull with code like that.\r\n\r\nAnother \"misuse\" example can be found at `containers/Dashboard/index.js`\r\nThe way you are handling the requests is not the graphql way. You should take advantage of the `graphql` HOC and you can then get rid of code like fetching on `componentDidMount`.\r\n\r\n## Axios\r\nno reason for this, drop in favor of fetch, which is a standard API.\r\n\r\n## File size\r\nTake `containers/Dashboard/index.js` this file is way too big.\r\nYou are mixing styled compoents with React component.\r\nCreate a `styled.js` file and put all the styled components there, so they are not in the way.\r\n\r\nThere are also route definitions, some of the state is missing in type annotations and it is hard to follow.\r\nThis needs to be split into multiple files.\r\n\r\n## Imports\r\ninstead of this\r\n`import LibraryDetail from \"../../containers/LibraryDetail\";`\r\nadd `.env` file with `NODE_PATH=src/` it will allow you to import it like this `\"containers/LibraryDetail\"`\r\n\r\n## Routing\r\nIdeally you would have `/pages` folder that would contain all the top level route components. In each of these folders you would have components/containers specific to this page. The shared ones would live in `/components` or `/containers` this is extremely important for project to be able to grow and still remain maintainable.\r\n\r\nAlso I would suggest to create `/pages/urls.js` file where you would define all the routes. That way you can get rid of magic constant urls and a potential to break links.\r\n\r\n## More on structure\r\nWhat you have in `/data` is okay, but i would suggest to break it down per entity.\r\n\r\nSo you would end up with\r\n```\r\n/data\r\n  /Animal\r\n    /apis.js\r\n    /groups.js\r\n    /helpers.js\r\n    /queries.js\r\n  /Books\r\n    /apis.js\r\n    /groups.js\r\n    /helpers.js\r\n    /queries.js\r\n```\r\nthis again, ensures your files are not too big and easy to follow.\r\n\r\n# What you can do with apollo, some inspiration from awayco, because i have been playing with GQL for quite a while there.\r\n`src/types/graphql.js`\r\nwhat a Query looks like, Data is type of data returned from the HOC, if the query takes params, they are the second type argument\r\n```\r\nexport type Query<Data, Params = empty> = {|\r\n  gql: any,\r\n  selector?: string[],\r\n  transform?: any => Data,\r\n  pagination?: string[],\r\n  options?: Function,\r\n|}\r\n```\r\n\r\n`src/data/reservations/graphql/queries.js`\r\nHere is then a concrete instance of the Query type\r\n```\r\nimport gql from \"graphql-tag\"\r\nimport type { Query } from \"types/graphql\"\r\n\r\nexport const reservationCountQuery: Query<number> = {\r\n  gql: gql`\r\n    query reservationCountQuery {\r\n      reservation {\r\n        list(filter: { limit: 9999999, status: confirmed }) {\r\n          data {\r\n            id\r\n          }\r\n        }\r\n      }\r\n    }\r\n  `,\r\n  transform: length,\r\n  selector: [\"reservation\", \"list\", \"data\"],\r\n}\r\n```\r\n\r\nI extended the behavior of the `graphql` HOC, so this code is quite complicated, but it allows for very easy use, ill show below.\r\n```\r\n// @flow\r\n// import * as React from \"react\"\r\nimport { graphql } from \"react-apollo\"\r\nimport type { Query } from \"types/graphql\"\r\nimport { compose, branch, renderComponent, type HOC } from \"recompose\"\r\nimport { getRouterVariables } from \"common/graphql/routing\"\r\nimport { transformResponse } from \"common/graphql/helpers\"\r\nimport withLoader from \"hoc/withLoader\"\r\nimport withEmpty from \"hoc/withEmpty\"\r\nimport NotFoundPage from \"pages/NotFound\"\r\nimport { pathSatisfies, pathOr, endsWith, find, always } from \"ramda\"\r\nimport { getPageInfo, getPaginationVariables, mergeData } from \"./pagination\"\r\n\r\nconst notFound = compose(\r\n  find(pathSatisfies(endsWith(\"NOT_FOUND\"), [\"data\", \"code\"])),\r\n  pathOr([], [\"error\", \"graphQLErrors\"]),\r\n)\r\n\r\n// eslint-disable-next-line\r\ntype Config<Outter, Variables> = {\r\n  noLoader?: boolean,\r\n  noEmpty?: boolean,\r\n  variables?: Outter => Variables,\r\n  config?: {\r\n    skip: Outter => boolean,\r\n  },\r\n}\r\n\r\ntype Added<Data> = {\r\n  data: Data,\r\n  loading: boolean,\r\n  loadingMore: boolean,\r\n  count: number,\r\n  loadMore: void => void,\r\n  notFound: boolean,\r\n}\r\n\r\nfunction withQuery<Outter, Data, Variables>(\r\n  query: Query<Data, Variables>,\r\n  configuration: Config<Outter, Variables> = {},\r\n  // $ExpectError\r\n): HOC<{ ...$Exact<Added<Data>>, ...$Exact<Outter> }, Outter> {\r\n  const { gql, selector, transform, pagination, options = always({}) } = query\r\n  const {\r\n    noLoader = false,\r\n    noEmpty = false,\r\n    variables = always({}),\r\n    config = {},\r\n  } = configuration\r\n\r\n  const customGraphqlHoc = graphql(gql, {\r\n    props: props => ({\r\n      data: transformResponse(selector, transform)(props),\r\n      // loading | setVariables | refetch\r\n      loading:\r\n        props.data.networkStatus === 1 ||\r\n        props.data.networkStatus === 2 ||\r\n        props.data.networkStatus === 4,\r\n      loadingMore: props.data.networkStatus === 3,\r\n      notFound: notFound(props.data),\r\n      count: pagination && getPageInfo(pagination, props.data).totalCount,\r\n      loadMore: () =>\r\n        pagination &&\r\n        !props.data.loading &&\r\n        getPageInfo(pagination, props.data).hasNextPage &&\r\n        props.data.fetchMore({\r\n          variables: getPaginationVariables(pagination, props.data),\r\n          updateQuery: (prev, { fetchMoreResult }) =>\r\n            mergeData(prev, fetchMoreResult, pagination),\r\n        }),\r\n    }),\r\n    options: props => ({\r\n      notifyOnNetworkStatusChange: true, // to update loading when fetching more\r\n      variables: {\r\n        ...getRouterVariables(props),\r\n        ...getPaginationVariables(pagination),\r\n        ...variables(props),\r\n      },\r\n      ...options(props),\r\n    }),\r\n    ...config,\r\n  })\r\n\r\n  return compose(\r\n    customGraphqlHoc,\r\n    ...(noLoader ? [] : [withLoader(props => props.loading)]),\r\n    branch(x => x.notFound, renderComponent(NotFoundPage)),\r\n    ...(noEmpty ? [] : [withEmpty()]),\r\n  )\r\n}\r\n\r\nexport default withQuery\r\n```\r\n\r\nAnd finally, how very easily it can be used\r\n```\r\n// @flow\r\nimport React from 'react'\r\nimport type { HOC } from 'recompose'\r\nimport { NavLink } from \"componentsStyled/Typography\"\r\nimport withQuery from \"hoc/withQuery\"\r\nimport { isNil } from \"ramda\"\r\nimport { reservationCountQuery } from \"data/reservations/graphql/queries\"\r\nimport urls from \"pages/urls\"\r\n\r\nconst ReservationsLink = ({ data }) => (\r\n  <NavLink to={urls.reservations}>\r\n    reservations&nbsp;{isNil(data) ? \"\" : `(${data})`}\r\n  </NavLink>\r\n)\r\n\r\nconst enhancer:HOC<*, {}> = withQuery(reservationCountQuery, {\r\n  noEmpty: true,\r\n  noLoader: true,\r\n})\r\n\r\nexport default enhancer(ReservationsLink)\r\n```\r\n\r\nI defined something similar for mutations, which then allows for a seamless refetching of queries and so on."},"comment":{"url":"https://api.github.com/repos/strvcom/dep-manager-web/issues/comments/440647921","html_url":"https://github.com/strvcom/dep-manager-web/issues/12#issuecomment-440647921","issue_url":"https://api.github.com/repos/strvcom/dep-manager-web/issues/12","id":440647921,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0MDY0NzkyMQ==","user":{"login":"bsunderhus","id":5483269,"node_id":"MDQ6VXNlcjU0ODMyNjk=","avatar_url":"https://avatars1.githubusercontent.com/u/5483269?v=4","gravatar_id":"","url":"https://api.github.com/users/bsunderhus","html_url":"https://github.com/bsunderhus","followers_url":"https://api.github.com/users/bsunderhus/followers","following_url":"https://api.github.com/users/bsunderhus/following{/other_user}","gists_url":"https://api.github.com/users/bsunderhus/gists{/gist_id}","starred_url":"https://api.github.com/users/bsunderhus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bsunderhus/subscriptions","organizations_url":"https://api.github.com/users/bsunderhus/orgs","repos_url":"https://api.github.com/users/bsunderhus/repos","events_url":"https://api.github.com/users/bsunderhus/events{/privacy}","received_events_url":"https://api.github.com/users/bsunderhus/received_events","type":"User","site_admin":false},"created_at":"2018-11-21T12:36:54Z","updated_at":"2018-11-21T12:36:54Z","author_association":"CONTRIBUTOR","body":"Ok let me add a list here of what has been done and is still missing\r\n\r\n- [x] SVG\r\n- [x] CSS reset (5cef36c15ffcaf63e2da3411ef47110b3a2f7c3a)\r\n- [x] Flow\r\n- [x] Apollo\r\n- [ ] Axios\r\n- [ ] File size\r\n- [x] Routing\r\n- [x] More on structure\r\n\r\n## SVGs (be77a6d381b81d32dea20429d0e51f78e77fda85)\r\n\r\nThe [create-react-app](https://facebook.github.io/create-react-app/) handles this by using [svgr](https://github.com/smooth-code/svgr) under the hood, transforming the SVG into a React component and also applying all optimizations on it.\r\n\r\n## Flow\r\n\r\nThe whole application has been converted to typescript from this commit 48a5d11e770ff739a4c258e81b9479be1a5e6902 forward. Most of the problems with the lack of good typings are still being fixed.\r\n\r\n## Apollo\r\n\r\nThe whole Apollo structure used in this project has been reviewed and now the [Apollo Client](https://www.apollographql.com/docs/react/) serves as a single source of truth for the whole project.\r\n\r\nThis has been quite a challenge to the project since it not only analyses the GitHub GraphQL API but also analyses the NPM API through [npms](https://api-docs.npms.io).\r\n\r\n## Axios\r\n\r\nThis is in the process of being completely removed!\r\n\r\n## File Size\r\n\r\nMost of the huge files were already broken into small ones, such as `Dashboard`, but still, some files are in the process of refactoring\r\n\r\n## Routing\r\n\r\nThe whole routing of the application has been remodeled to take bigger advantages of redirections, switches, and conditional routing.\r\n"}}
{"actor":{"display_login":"bsunderhus"}}