search

ibnukipa / my-git-app

MyGit App
0 stars 0 forks source link

Not an issue actually, but questions. #1

Open fachri-adakerja opened 2 years ago

fachri-adakerja commented 2 years ago

Hi @ibnukipa :wave:

I'm Fachri, Software Engineer at AdaKerja. Thanks for the submission. It looks great :fire: and Sorry if you waiting for our response.

Let's discuss something in my-git-app.

  1. What's eslint . doing in the package.json script?
  2. On this file useBackdropBS.js, why did you create react hooks to render a backdrop instead of creating it as a component? I just curious about that
  3. Have you heard about redux-saga? if you don't mind, could you improve your submission to using redux-saga?
  4. Is there any reason you not creating any unit test?

cc: @ashwintiwari88 @ivan-adakerja

ibnukipa commented 2 years ago

Hi @fachri-adakerja , thanks for the feedback! I would like to answer the questions.

  1. the eslint . is mean to be code assistant for code styling and analyze possible issue.
  2. actually the hook is enhancement for https://github.com/gorhom/react-native-bottom-sheet which not all bottom sheet will having the backdrop, so the renderBackdrop will be used for bottom sheet that needs a backdrop. I prefer to use hook rather than HOC or compose it to a Component because I only have one functionality of enhancement.
  3. in my current design and current feature, there is no side-effect yet so there is no need a side-effect (redux-saga, redux-thunk, or redux-observable) middleware for redux
  4. I missed the testing part, do you prefer e2e testing (detox) or unit testing (RNTL) ?
fachri-adakerja commented 2 years ago

Hi @ibnukipa

  1. great, have you run your lint command? 
➜  my-git-app git:(master) yarn && yarn lint
yarn install v1.22.11
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
warning "react-native > react-native-codegen > jscodeshift@0.11.0" has unmet peer dependency "@babel/preset-env@^7.1.6".
warning " > redux-flipper@2.0.1" has unmet peer dependency "redux@^4".
warning " > redux-persist@6.0.0" has unmet peer dependency "redux@>4.0.0".
warning "@react-native-community/eslint-config > @typescript-eslint/eslint-plugin > tsutils@3.21.0" has unmet peer dependency "typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta".
[4/4] 🔨  Building fresh packages...

✨  Done in 43.23s.
yarn run v1.22.11
$ eslint .
jsxBracketSameLine is deprecated.

/Volumes/Data/Projects/my-git-app/src/screens/CommitScreen.js
  172:6  error  React Hook useEffect has a missing dependency: 'refresh'. Either include it or remove the dependency array     react-hooks/exhaustive-deps
  217:6  error  React Hook useEffect has a missing dependency: 'navigation'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/Volumes/Data/Projects/my-git-app/src/screens/DashboardScreen.js
  11:9   error    Replace `·useDispatch,·useSelector·}·from·"react-redux"` with `useDispatch,·useSelector}·from·'react-redux'`                       prettier/prettier
  11:42  warning  Strings must use singlequote                                                                                                       quotes
  12:9   error    Replace `·authStateSelector·}·from·"../states/reducers/auth"` with `authStateSelector}·from·'../states/reducers/auth'`             prettier/prettier
  12:35  warning  Strings must use singlequote                                                                                                       quotes
  40:43  error    Replace `·repoOwner,·repoSlug,·token:·authState.token` with `⏎······repoOwner,⏎······repoSlug,⏎······token:·authState.token,⏎···`  prettier/prettier
  52:6   error    React Hook useCallback has a missing dependency: 'authState.token'. Either include it or remove the dependency array               react-hooks/exhaustive-deps

✖ 8 problems (6 errors, 2 warnings)
  3 errors and 2 warnings potentially fixable with the `--fix` option.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

  1. I still don't get the idea, but that's fine, not a big deal 👌

  2. I think API requests are counted as a side effect. ref: https://smartcar.com/blog/what-the-react-sagas

  3. Unit tests are okay