Change eslint configuration and fix resulting issues #4423

arikfr · 2019-12-05T14:25:42Z

What type of PR is this?

Other

Description

Review the automatic changes to dependencies @gabrieldutra @kravets-levko
Fix the value setting in useEffect @gabrieldutra @kravets-levko

We were using airbnb's eslint settings, which at the time seemed like a good setting to use. The problem was that they were outdated and had a mix of styling rules (like which type of quotes to use or line length) and actual linting rules. We are also using Prettier to format our code and it has some conflicting style choices with the eslint rules.

While this can be resolved by using prettier-eslint, it's less recommended these days. I decided to find us better defaults that will be more about linting and les about style. In search of a better default, I decided to pick create-react-app's eslint configuration, as it's very likely we will adopt create-react-app once we get rid of Angular..js in our codebase.

(later I found eslint-config-prettier, which just disables the conflicting rules, but already invested the work in doing the switch)

This resulted in a few new rules that found some issues in our codebase:

Issues with dependencies provided to React Hooks - 0d4730d. This was fixed with eslint --fix. @gabrieldutra @kravets-levko can you make sure it all makes sense?
Mixed operators issues- cf6d780. I actually found it confusing myself.
The rule that checks useCallback's dependencies found an actual bug in our code:
fd7d19a.
6c676f9: another bug found by a lint rule.

I also added Prettier config and removed some dead code, but I will apply Prettier on the codebase in a separate PR to make it easier the code changes in this PR.

There were 3 issues that I wasn't sure how to fix, although they did seem like an actual issue:

client/app/components/QuerySelector.jsx
  67:23  warning  Assignments to the 'isStaleSearch' variable from inside React Hook useEffect will be lost after each render. To preserve the value over time, store it in a useRef Hook and keep the mutable value in the '.current' property. Otherwise, you can move this variable directly inside useEffect  react-hooks/exhaustive-deps

client/app/lib/hooks/useQueryResult.js
  27:21  warning  Assignments to the 'isCancelled' variable from inside React Hook useEffect will be lost after each render. To preserve the value over time, store it in a useRef Hook and keep the mutable value in the '.current' property. Otherwise, you can move this variable directly inside useEffect  react-hooks/exhaustive-deps

client/app/lib/hooks/useSearchResults.js
  29:27  warning  Assignments to the 'isDestroyed' variable from inside React Hook useEffect will be lost after each render. To preserve the value over time, store it in a useRef Hook and keep the mutable value in the '.current' property. Otherwise, you can move this variable directly inside useEffect  react-hooks/exhaustive-deps

…ake sure hooks are called in the same order.

…uch global.

kravets-levko · 2019-12-05T14:35:55Z

client/app/components/app-header/components/FavoritesDropdown.jsx

@@ -27,7 +27,7 @@ export default function FavoritesDropdown({ fetch, urlTemplate }) {
  }, [fetch]);

  // fetch items on init
-  useEffect(() => fetchItems(false), []);
+  useEffect(() => fetchItems(false), [fetchItems]);


Are you sure this should be fixed? According to comment, it should fetch items only once, but with this fix it will re-load items each time fetchItems changed

Not sure at all - it's part of the automatic fixes.

I'm also not sure how it supposed to work. Maybe @gabrieldutra will help 🤔

I'm testing these eslint changes, it's indeed a case where it makes it mandatory. In this specific case I think it's ok to keep the dependency as fetchItems should only change if the fetch prop change (in this case I think we do want the items to be loaded again).

Though from a quick search there are discussions around this (like this one). Just need to confirm if there's a more correct React way (they do mention useMemo, but imo that only applies to when you don't have a Promise to solve).

A side note I just noticed: I'd avoid the () => executeFunction() for useEffect as the return value is used for cleaning. Anyway, I'll check other replacements to see if they can be problematic.

kravets-levko · 2019-12-05T14:36:46Z

client/app/components/permissions-editor/PermissionsEditorDialog.jsx

@@ -34,7 +34,7 @@ function useGrantees(url) {

  const addPermission = useCallback((userId, accessType = 'modify') => $http.post(
    url, { access_type: accessType, user_id: userId },
-  ).catch(() => notification.error('Could not grant permission to the user'), [url]));
+  ).catch(() => notification.error('Could not grant permission to the user')), [url]);


kravets-levko · 2019-12-05T14:38:03Z

client/app/components/permissions-editor/PermissionsEditorDialog.jsx

@@ -77,7 +77,7 @@ function UserSelect({ onSelect, shouldShowUser }) {
  useEffect(() => {
    setLoadingUsers(true);
    debouncedSearchUsers(searchTerm);
-  }, [searchTerm]);
+  }, [debouncedSearchUsers, searchTerm]);


Probably it should be replaced with use-debounced 🤔 But let's keep as is for now

kravets-levko · 2019-12-05T14:43:14Z

client/app/visualizations/choropleth/Renderer/index.jsx

@@ -58,7 +58,7 @@ export default function Renderer({ data, options, onOptionsChange }) {
        options, // detect changes for all options except bounds, but pass them all!
      );
    }
-  }, [map, geoJson, data, optionsWithoutBounds]);
+  }, [map, geoJson, data, optionsWithoutBounds, options]);


This change will cause performance issues - see comment above

@gabrieldutra Can you please address this in #4425 as well?

kravets-levko · 2019-12-05T14:45:13Z

client/app/visualizations/funnel/Renderer/index.jsx

@@ -16,7 +16,7 @@ function generateRowKeyPrefix() {

 export default function Renderer({ data, options }) {
  const funnelData = useMemo(() => prepareData(data.rows, options), [data, options]);
-  const rowKeyPrefix = useMemo(() => generateRowKeyPrefix(), [funnelData]);
+  const rowKeyPrefix = useMemo(() => generateRowKeyPrefix(), []);


rowKeyPrefix should be generated every time funnelData changes to ensure that Ant Table will properly update rows

I was checking, useEffect doesn't require dependencies to be used (useMemo does), so the options: 1 - Move it to useEffect and useState for the rowKeyPrefix; 2 - eslint-disable;

I think 2 is the quick one ^^

1 is more confusing (IMO) - this use case is perfect for useMemo, but not as obvious with useState + useEffect. So I'm for 2, or I can try to find another (?) solution.

gabrieldutra · 2019-12-05T17:01:51Z

package.json

-    "eslint": "^5.16.0",
-    "eslint-config-airbnb": "^17.1.0",
-    "eslint-import-resolver-webpack": "^0.8.3",
-    "eslint-loader": "^2.1.1",


Isn't this necessary to load eslint with webpack? I started getting an error for it (webpack.config.js points to eslint-loader)

Just confirming that, I'll open a PR with some updates, if you want I can do that along with it.

My bad... I just removed all the previous *eslint* dependencies 😬

gabrieldutra · 2019-12-07T21:57:01Z

@kravets-levko regarding the other 3 issues Arik mentioned (the let variables to avoid stale results), from what I understood it's meant to be like that, but has an issue ever happened with the nonexistence of that? I mean, do we really need that, or, isn't the unsubscribe function in useEffect enough?

arikfr · 2019-12-08T08:18:32Z

@gabrieldutra I'm not sure if an issue will be noticeable or easy to reproduce. But reading the error + the code it does seem like the code that uses these let variables will not work.

kravets-levko · 2019-12-08T10:37:48Z

client/app/lib/hooks/useQueryResult.js
27:21 warning Assignments to the 'isCancelled' variable from inside React Hook useEffect will be lost after each render.

isCancelled should be moved to useEffect

client/app/lib/hooks/useSearchResults.js
29:27 warning Assignments to the 'isDestroyed' variable from inside React Hook useEffect will be lost after each render.

isDestroyed should be replaced with useRef

client/app/components/QuerySelector.jsx
67:23 warning Assignments to the 'isStaleSearch' variable from inside React Hook useEffect will be lost after each render

The perfect solution is to use useSearchResults (with fix mentioned above). Also, I'm not sure if that code should use useRef instead of let 🤔

gabrieldutra

Also, I'm not sure if that code should use useRef instead of let

Seemed the best option to solve this 👍.

~~BTW, I didn't see a reason to refactor that to use useSearchResults, so I just applied the same useRef fix as it was quicker.~~ I realized that the useRef in that file causes issues, so moved to use useSearchResults.

gabrieldutra · 2019-12-08T22:30:57Z

client/app/visualizations/choropleth/Renderer/index.jsx

@@ -58,7 +58,7 @@ export default function Renderer({ data, options, onOptionsChange }) {
        options, // detect changes for all options except bounds, but pass them all!
      );
    }
-  }, [map, geoJson, data, optionsWithoutBounds]);
+  }, [map, geoJson, data, optionsWithoutBounds, options]);


arikfr · 2019-12-10T15:58:00Z

@kravets-levko good to go?

kravets-levko

Seems everything is fine 👍

arikfr added 11 commits December 5, 2019 15:28

Remove app/service/query-string (unused) and its dependency.

6701516

Fix usage of mixed operators.

cf6d780

eslint --fix fixes for missing dependencies for react hooks

0d4730d

Fix: useCallback dependency passed to $http's .catch.

fd7d19a

Satisfy react/no-direct-mutation-state.

619baf4

Fix no-mixed-operators violations.

01c9e36

Move the decision of whether to render Custom chart one level up to m…

32e4a7e

…ake sure hooks are called in the same order.

Fix: name was undefined. It wasn't detected before because there is s…

6c676f9

…uch global.

Simplify eslint config and switch to creat-react-app's eslint base.

c207ec0

Add prettier config.

b25c229

Make sure eslint doesn't conflict with prettier

3d6665b

arikfr added Frontend Frontend: React Frontend codebase migration to React labels Dec 5, 2019

arikfr requested review from gabrieldutra and kravets-levko December 5, 2019 14:25

kravets-levko reviewed Dec 5, 2019

View reviewed changes

gabrieldutra reviewed Dec 5, 2019

View reviewed changes

gabrieldutra reviewed Dec 8, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Dec 9, 2019

Weekly Digest (2 December, 2019 - 9 December, 2019) #4427

Closed

A few updates post eslint (#4425)

5936249

gabrieldutra approved these changes Dec 9, 2019

View reviewed changes

Prettier command in package.json

5be44d2

kravets-levko approved these changes Dec 10, 2019

View reviewed changes

arikfr merged commit 1b9b303 into master Dec 11, 2019

arikfr deleted the eslint branch December 11, 2019 10:00

arikfr mentioned this pull request Dec 11, 2019

Prettier all the Javascript code & GitHub Action #4433

Merged

3 tasks

gabrieldutra mentioned this pull request Dec 11, 2019

Restyle Migrate schema browser to react #4436

Closed

weekly-digest bot mentioned this pull request Dec 16, 2019

Weekly Digest (9 December, 2019 - 16 December, 2019) #4451

Closed

gabrieldutra mentioned this pull request Feb 18, 2020

Fix Add Widget always shows recent queries list #4658

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change eslint configuration and fix resulting issues #4423

Change eslint configuration and fix resulting issues #4423

arikfr commented Dec 5, 2019 •

edited

Loading

kravets-levko Dec 5, 2019

arikfr Dec 5, 2019

kravets-levko Dec 5, 2019

gabrieldutra Dec 5, 2019

kravets-levko Dec 5, 2019

kravets-levko Dec 5, 2019

kravets-levko Dec 5, 2019

kravets-levko Dec 8, 2019

gabrieldutra Dec 8, 2019

kravets-levko Dec 5, 2019

gabrieldutra Dec 6, 2019

kravets-levko Dec 6, 2019

gabrieldutra Dec 5, 2019

gabrieldutra Dec 5, 2019

arikfr Dec 8, 2019

gabrieldutra commented Dec 7, 2019

arikfr commented Dec 8, 2019

kravets-levko commented Dec 8, 2019

gabrieldutra left a comment •

edited

Loading

gabrieldutra Dec 8, 2019

arikfr commented Dec 10, 2019

kravets-levko left a comment

Change eslint configuration and fix resulting issues #4423

Change eslint configuration and fix resulting issues #4423

Conversation

arikfr commented Dec 5, 2019 • edited Loading

What type of PR is this?

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Dec 7, 2019

arikfr commented Dec 8, 2019

kravets-levko commented Dec 8, 2019

gabrieldutra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr commented Dec 10, 2019

kravets-levko left a comment

Choose a reason for hiding this comment

arikfr commented Dec 5, 2019 •

edited

Loading

gabrieldutra left a comment •

edited

Loading