Code review 12.12.2018

[petrhanak]

Overall view on the project is positive, code is nicely structured, no big files or mess in codebase. Bonus points are for using React hooks, Typescript. Apollo and Ramda - it’s good fit for this kind of internal project.

Issues

There is not much to point out, but still there is one issue that must be fixed.

1. Starting project is not possible by only git clone > nom install > nom start. For frontend projects .env file(s) should be committed as values in those are not actual secrets. See Dan Abramovs comment for more info: https://github.com/facebook/create-react-app/issues/2403#issuecomment-304624540


Enhancements

Here is few minor issues that would be nice to get rid of:

2. In package.json, tslint.json and tsconfig.json are several packages/rules that are unnecessary or shouldn’t be used.

   packages.json

• apollo-boost
• apollo-link-context
• axios
• date-fns
• prop-types
• recompose
• shallow-equal
• uuid
• @storybook/addon-actions
• @storybook/addon-links
• classnames

tslint.json

• no-implicit-dependencies
• no-unused-variables

tsconfig.json

• removeComments
• declaration
• noImplicitReturns
• experimentalDecorators
• lib: ["es5","es6"]
• allowUnreachableCode
• diagnostics
• listEmittedFiles
• stripInternal

3. Config values like process.env.REACT_APP_… shouldn’t be used directly. Rather import them from shared config.js file.

4. Get rid of relative imports ../../... Add babel-plugin-module-resolver and paths in tsconfig. Here is example config:


.babelrc

{
  "plugins": [
    ["module-resolver", {
      "alias": {
        "~": "./src"
      }
    }]
  ]
}


tsconfig.json


{
  "compilerOptions": {
    "paths": {
      "~/*": ["./src/*"]
    }
}

5. Few more adjustments can be made in .babelrc:


• babel-plugin-ramda enables imports like import { map } from 'ramda’- no need to write ramda/es/map :)

• babel-plugin-styled-components will provide display name for better debugging.

6. There is not many styles yet, but I recommend adding stylelint

7. In precommit hook it would be nice to run linters too (tslint, stylelint) also it’s nice to have lint-staged & tsc in case there is error.


Summary

👍 Using React hooks
👍 Very clean codebase
〰️ Bloated package.json. tsconfigand tslint
〰️ env values used directly
👎 Project needs additional configuration which is not provided - missing .env

[bsunderhus]

Thanks for the review @petrhanak! Appreciated!!

The thing about the .env was a choice made previously that I erroneously kept. Thanks for the heads up, I'll be putting a PR to fix everything you pointed!

[bsunderhus]

Apparently the create-react-app overrides my tsconfig.json file. I tried to add the paths but couldn't, every time I run the development server with npm start it simply overrides every modification I did.

There's an open issue for this.